{"thread":{"id":"31349","subject":"git no longer prompting for password","startedAt":"2012-08-24T20:19:28Z","lastAt":"2012-08-28T18:06:52Z","messageCount":22,"participants":["Iain Paton","Jeff King","BJ Hargrave","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"197793","messageId":"5037E1D0.6030900@gmail.com","threadId":"31349","inReplyTo":null,"subject":"git no longer prompting for password","fromName":"Iain Paton","fromEmail":"ipaton0@gmail.com","sentAt":"2012-08-24T20:19:28Z","receivedAt":"2012-08-24T20:19:28Z","isPatch":false,"sender":{"key":"ipaton0@gmail.com","avatar":null},"body":"Hi List,\n\nA 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.\n\nThe 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.\n\nBacktracking 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.\n\nuser@fubar01:~/test# git --version\ngit version 1.7.7.7\nuser@fubar01:~/test# git push http://ipaton@10.0.0.1/git/test.git master\nPassword: \n\ntype the password in and the push is successful\n\nuser@fubar01:~/test# git --version\ngit version 1.7.8\nuser@fubar01:~/test# git push http://ipaton@10.0.0.1/git/test.git master --verbose\nPushing to http://ipaton@10.0.0.1/git/test.git\nCounting objects: 6, done.\nDelta compression using up to 8 threads.\nCompressing objects: 100% (3/3), done.\nWriting objects: 100% (5/5), 491 bytes, done.\nTotal 5 (delta 0), reused 0 (delta 0)\nerror: RPC failed; result=22, HTTP code = 401\nfatal: The remote end hung up unexpectedly\nfatal: The remote end hung up unexpectedly\n\nWatching 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..\n\ngoogling 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!  \nLooking at changelogs for 1.7.8 and I'm not really seeing anything that says I need to do something different.\n\nAny help or pointers appreciated.\n\nThanks,\nIain\n"},{"id":"197795","messageId":"20120824212501.GA16285@sigill.intra.peff.net","threadId":"31349","inReplyTo":"5037E1D0.6030900@gmail.com","subject":"Re: git no longer prompting for password","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-08-24T21:25:01Z","receivedAt":"2012-08-24T21:25:01Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Aug 24, 2012 at 09:19:28PM +0100, Iain Paton wrote:\n\n> A recent update to git 1.7.12 from 1.7.3.5 seems to have changed\n> something - trying to push to a smart http backend no longer prompts\n> for a password and hence fails the server auth.\n> [...]\n> Backtracking through the versions I've skipped and this doesn't seem\n> to be a new problem, client side up to 1.7.7.7 works, 1.7.8 onwards\n> don't. Server side version doesn't seem to make a difference.\n\nThere was some work in v1.7.8 to avoid prompting for a password when it\nis not necessary; I suspect this is a fallout of that.\n\nYou could try bisecting the bug. My guess is that you will end up at\ncommit 986bbc0 (http: don't always prompt for password, 2011-11-04).\n\n> user@fubar01:~/test# git --version\n> git version 1.7.7.7\n> user@fubar01:~/test# git push http://ipaton@10.0.0.1/git/test.git master\n> Password: \n\nAs per the discussion in 986bbc0, this is actually prompting you before\ngit makes any request. Whereas here:\n\n> user@fubar01:~/test# git --version\n> git version 1.7.8\n> user@fubar01:~/test# git push http://ipaton@10.0.0.1/git/test.git master --verbose\n\nWe should get an HTTP 401 from the server, then prompt, then retry.\nWhat's weird is that it sort of works:\n\n> Pushing to http://ipaton@10.0.0.1/git/test.git\n> Counting objects: 6, done.\n> Delta compression using up to 8 threads.\n> Compressing objects: 100% (3/3), done.\n> Writing objects: 100% (5/5), 491 bytes, done.\n> Total 5 (delta 0), reused 0 (delta 0)\n> error: RPC failed; result=22, HTTP code = 401\n> fatal: The remote end hung up unexpectedly\n> fatal: The remote end hung up unexpectedly\n\nIt's like the initial http requests do not get a 401, and the push\nproceeds, and then some later request causes a 401 when we do not expect\nit. Which is doubly odd, since we should also be able to handle that\ncase (the first 401 we get should cause us to ask for a password).\n\nCan you show us the result of running with GIT_CURL_VERBOSE=1? I'd\nreally like to see which requests are being made with and without\nauthentication.\n\n> Looking at changelogs for 1.7.8 and I'm not really seeing anything\n> that says I need to do something different.\n\nNo, you shouldn't need to do anything different. I'd suspect the\nweirdness you are seeing is from a credential helper trying to supply a\nblank password, except that you would have to have configured one\nmanually for it to run (I assume you are not on a shared machine where\nsomebody might have tweaked /etc/gitconfig or anything like that).\n\n-Peff\n"},{"id":"197848","messageId":"20120825203904.GA10470@sigill.intra.peff.net","threadId":"31349","inReplyTo":"5038E781.1090008@gmail.com","subject":"Re: git no longer prompting for password","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-08-25T20:39:05Z","receivedAt":"2012-08-25T20:39:05Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Aug 25, 2012 at 03:56:01PM +0100, Iain Paton wrote:\n\n> > It's like the initial http requests do not get a 401, and the push\n> > proceeds, and then some later request causes a 401 when we do not expect\n> > it. Which is doubly odd, since we should also be able to handle that\n> > case (the first 401 we get should cause us to ask for a password).\n> \n> Yes, I deliberately have it set for anonymous pull and authenticated push. \n> So the initial contact with the server doesn't ask for auth.\n\nOK, I see what's going on. It looks like it is configured to do so by\nrejecting the POST request. So this first request works:\n\n> > GET /git/test.git/info/refs?service=git-receive-pack HTTP/1.1\n> User-Agent: git/1.7.8\n> Host: 10.44.16.74\n> Accept: */*\n> Pragma: no-cache\n> \n> < HTTP/1.1 200 OK\n\nwhich is the first step of the conversation, in which the client gets\nthe set of refs from the remote. Then it tries to POST the pack:\n\n> > POST /git/test.git/git-receive-pack HTTP/1.1\n> User-Agent: git/1.7.8\n> Host: 10.44.16.74\n> Accept-Encoding: deflate, gzip\n> Content-Type: application/x-git-receive-pack-request\n> Accept: application/x-git-receive-pack-result\n> Content-Length: 412\n> \n> * upload completely sent off: 412 out of 412 bytes\n> < HTTP/1.1 401 Unauthorized\n\nAnd we get blocked on that request. I didn't quote it above, but note\nhow the client actually generates and sends the full pack before being\ntold \"no, you can't do this\".\n\nSo that explains the output you see; we really are generating and\nsending the pack, and only then getting a 401. And it also explains why\ngit does not prompt and retry; we follow a different code path for POSTs\nthat does not trigger the retry code.\n\nThis is not optimal, as we send the pack data only to find out that we\nare not authenticated. There is code to avoid sending the _whole_ pack\n(it's the probe_rpc code in remote-curl.c), so I think you'd just be\nwasting 64K, which is not too bad. So we could teach git to retry if the\nPOST fails, and I think it would work OK.\n\nBut I don't think there is any reason not to block the push request\nright from the first receive-pack request we see, which catches the\nissue even earlier, and with less overhead (and of course works with\nexisting git clients :) ).\n\n> apache config has the following:\n> [...]\n> <LocationMatch \"^/git/.*/git-receive-pack$\">\n>         AuthType Basic\n>         AuthUserFile /data/git/htpasswd\n>         AuthGroupfile /data/git/groups \n>         AuthName \"Git Access\"\n> \n>         Require group committers\n> </LocationMatch>\n> \n> nothing untoward there I think and google turns up lots of examples where \n> people are doing essentially the same thing.\n\nI think your regex is the culprit. The first request comes in with:\n\n> > GET /git/test.git/info/refs?service=git-receive-pack HTTP/1.1\n\nThe odd URL is because we are probing to see if the server even supports\nsmart-http. But note that it does not match your regex above, which\nrequires \"/git-receive-pack\". It looks like that is pulled straight from\nthe git-http-backend manpage. I think the change in v1.7.8 broke people\nusing that configuration.\n\nI tend to think the right thing is to fix the configuration (both on\nyour system and in the documentation), but we should probably also fix\ngit to handle this situation more gracefully, since it used to work and\nhas been advertised in the documentation for a long time.\n\n-Peff\n"},{"id":"197857","messageId":"5039F327.9010003@gmail.com","threadId":"31349","inReplyTo":"20120825203904.GA10470@sigill.intra.peff.net","subject":"Re: git no longer prompting for password","fromName":"Iain Paton","fromEmail":"ipaton0@gmail.com","sentAt":"2012-08-26T09:57:59Z","receivedAt":"2012-08-26T09:57:59Z","isPatch":false,"sender":{"key":"ipaton0@gmail.com","avatar":null},"body":"On 25/08/12 21:39, Jeff King wrote:\n\n> I think your regex is the culprit. The first request comes in with:\n> \n>>> GET /git/test.git/info/refs?service=git-receive-pack HTTP/1.1\n> \n> The odd URL is because we are probing to see if the server even supports\n> smart-http. But note that it does not match your regex above, which\n> requires \"/git-receive-pack\". It looks like that is pulled straight from\n> the git-http-backend manpage. I think the change in v1.7.8 broke people\n> using that configuration.\n\nYes, it was lifted straight out of the manpage, albeit a couple of years \nago now and there have been additions to the manpage since then. \nI did check, and the basic config is identical in the current manpage.\n\nI can't be the only one using a config that's based on the example in \nthe manpage surely ?  So I'm surprised this hasn't come up previously.\n\n\n> I tend to think the right thing is to fix the configuration (both on\n> your system and in the documentation), but we should probably also fix\n> git to handle this situation more gracefully, since it used to work and\n> has been advertised in the documentation for a long time.\n\nSo after some head scratching trying to work out how to do the equivalent of \nLocationMatch but on the query string I came up with the following:\n\nScriptAlias /git/ /usr/libexec/git-core/git-http-backend/\n\n<Directory /usr/libexec/git-core>\n        Require ip 10.44.0.0/16\n        <If \"%{THE_REQUEST} =~ /git-receive-pack/\">\n                AuthType Basic\n                AuthUserFile /data/git/htpasswd\n                AuthGroupfile /data/git/groups\n                AuthName \"Git Access\"\n\n                Require group committers\n        </If>\n</Directory>\n\nand I've removed the LocationMatch section completely.\n\nSo for accesses to git-http-backend I require auth if anything in the request \nincludes git-receive-pack and that causes a prompt for the username/password \nas required, while at the same time it still allows anonymous pull.\n\nIt appears that the clone operation uses\n\nGET /git/test.git/info/refs?service=git-upload-pack HTTP/1.1\n\nto probe for smart-http ?  So this would be ok ?\n\nI'm not sure this is ideal, I don't really know enough about the protocol to know \nif I'll see git-receive-pack elsewhere. Possibly if someone includes it in the \nname of a repo it'll blow up in my face.\nI can always change it to match only on QUERY_STRING and put the LocationMatch \nback in if that happens.\n\nIf that's all that's required, I'm fine with an easy change to httpd.conf\n\nThanks for the help Jeff.\n"},{"id":"197858","messageId":"20120826101341.GA12566@sigill.intra.peff.net","threadId":"31349","inReplyTo":"5039F327.9010003@gmail.com","subject":"Re: git no longer prompting for password","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-08-26T10:13:41Z","receivedAt":"2012-08-26T10:13:41Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Aug 26, 2012 at 10:57:59AM +0100, Iain Paton wrote:\n\n> > The odd URL is because we are probing to see if the server even supports\n> > smart-http. But note that it does not match your regex above, which\n> > requires \"/git-receive-pack\". It looks like that is pulled straight from\n> > the git-http-backend manpage. I think the change in v1.7.8 broke people\n> > using that configuration.\n> \n> Yes, it was lifted straight out of the manpage, albeit a couple of years \n> ago now and there have been additions to the manpage since then. \n> I did check, and the basic config is identical in the current manpage.\n> \n> I can't be the only one using a config that's based on the example in \n> the manpage surely ?  So I'm surprised this hasn't come up previously.\n\nYeah, I'm surprised it took this long to come up, too. Perhaps most\npeople just do anonymous http, and then rely on ssh for pushing to\nachieve the same effect. Or maybe my analysis of the problem is wrong.\n:)\n\nI'm preparing some patches to the test suite that will demonstrate the\nproblem (we test dumb-http auth, but we don't do any smart-http auth at\nall in the test suite), and then a fix on top to let us prompt for the\npassword in this instance. I think we should also update the\ndocumentation, but the existing advice has been given long enough that\npeople are going to use it for some time, and I consider your issue to\nbe a regression in v1.7.8 that should be fixed.\n\n> So after some head scratching trying to work out how to do the equivalent of \n> LocationMatch but on the query string I came up with the following:\n> \n> ScriptAlias /git/ /usr/libexec/git-core/git-http-backend/\n> \n> <Directory /usr/libexec/git-core>\n>         Require ip 10.44.0.0/16\n>         <If \"%{THE_REQUEST} =~ /git-receive-pack/\">\n>                 AuthType Basic\n>                 AuthUserFile /data/git/htpasswd\n>                 AuthGroupfile /data/git/groups\n>                 AuthName \"Git Access\"\n> \n>                 Require group committers\n>         </If>\n> </Directory>\n> \n> and I've removed the LocationMatch section completely.\n\nYeah, I think that will work. It feels a little weird and hacky. E.g.,\nwhat if you had a repo named git-receive-pack? Unlikely, of course, but\nI'd want the config we advertise in the manpage to be as robust as\npossible.\n\nI don't know enough about Apache to know off-hand if there is a cleaner\nway. I'll investigate a bit more before doing my documentation patch.\n\n> So for accesses to git-http-backend I require auth if anything in the request \n> includes git-receive-pack and that causes a prompt for the username/password \n> as required, while at the same time it still allows anonymous pull.\n> \n> It appears that the clone operation uses\n> \n> GET /git/test.git/info/refs?service=git-upload-pack HTTP/1.1\n> \n> to probe for smart-http ?  So this would be ok ?\n\nRight. Anything invoking receive-pack is always a push.\n\n> I'm not sure this is ideal, I don't really know enough about the protocol to know \n> if I'll see git-receive-pack elsewhere. Possibly if someone includes it in the \n> name of a repo it'll blow up in my face.\n\nYep, exactly. That should be the only place, though, I think (branch\nnames, for example, are never part of the URL).\n\n> I can always change it to match only on QUERY_STRING and put the LocationMatch \n> back in if that happens.\n\nI think that would be cleaner. It would be even nicer if you could\nreally just match \"service=\" as a query parameter, but I don't know that\napache parses that at all. I also don't know if Apache does any\ncanonicalization of the QUERY_STRING. When matching, you'd want to make\nsure there is no way of a client sneaking in a parameter that git would\nunderstand to mean a push, but that your pattern would not notice (so,\ne.g., just matching \"git-receive-pack$\" would not be sufficient, as I\ncould request \"?service=git-receive-pack&fooled_you=true\". I don't\nrecall whether git rejects nonsense like that itself.\n\n> If that's all that's required, I'm fine with an easy change to httpd.conf\n> \n> Thanks for the help Jeff.\n\nNo problem. I'll probably be a day or two on the patches, as the http\ntests are in need of some refactoring before adding more tests. But in\nthe meantime, I think your config change is a sane work-around.\n\n-Peff\n"},{"id":"197860","messageId":"503A3023.6000103@gmail.com","threadId":"31349","inReplyTo":"20120826101341.GA12566@sigill.intra.peff.net","subject":"Re: git no longer prompting for password","fromName":"Iain Paton","fromEmail":"ipaton0@gmail.com","sentAt":"2012-08-26T14:18:11Z","receivedAt":"2012-08-26T14:18:11Z","isPatch":false,"sender":{"key":"ipaton0@gmail.com","avatar":null},"body":"On 26/08/12 11:13, Jeff King wrote:\n\n> Yeah, I'm surprised it took this long to come up, too. Perhaps most\n> people just do anonymous http, and then rely on ssh for pushing to\n> achieve the same effect. Or maybe my analysis of the problem is wrong.\n> :)\n\nI'd be using ssh to push too, but the simple fact is that the http way \nworks through a proxy and so essentially works from anywhere. The same \nisn't true for ssh or git protocols. Well that's my reason anyway :)\n\n> Yeah, I think that will work. It feels a little weird and hacky. E.g.,\n\nYeah, it does. I couldn't find a simple way though, most stuff like \nLocationMatch specifically excludes the query string which makes it \nrather more difficult.\n\n> I don't know enough about Apache to know off-hand if there is a cleaner\n> way. I'll investigate a bit more before doing my documentation patch.\n\nI'm not an apache expert either. What I could find was using mod_rewrite to \nset an env var based on something in the query string, but not actually do \nany rewrite. Then looking at how to check the env var and do something based \non that got me the example of simply using If with an expression to match \ndirectly on the query string.\n\n> I think that would be cleaner. It would be even nicer if you could\n> really just match \"service=\" as a query parameter, but I don't know that\n> apache parses that at all. I also don't know if Apache does any\n> canonicalization of the QUERY_STRING. When matching, you'd want to make\n\n>From what I can tell apache really doesn't care much about the query string \nat all, it seems to just pass it through unless you start messing with it \nusing mod_rewrite, but even then you're still regex based. I couldn't find \nanything that parsed out individual parameters. Of course I could just be \nlooking in all the wrong places :) \n\n> sure there is no way of a client sneaking in a parameter that git would\n> understand to mean a push, but that your pattern would not notice (so,\n> e.g., just matching \"git-receive-pack$\" would not be sufficient, as I\n\nyep, and matching on THE_REQUEST gets you the whole string, including the \nHTTP/1.1 on the end. I tried putting the $ on the end of the regex and it \ndidn't work. \nIt should be possible to combine the original regex from the LocationMatch \nexample and something like /[?&]service=git-receive-pack/ though, which \nshould make it somewhat safer.\n\n> No problem. I'll probably be a day or two on the patches, as the http\n> tests are in need of some refactoring before adding more tests. But in\n> the meantime, I think your config change is a sane work-around.\n\nWorks-For-Me is all I need right now :)  I'll be interested if you come \nup with something better though.\n\nIain\n"},{"id":"197890","messageId":"503B2FA9.6030700@gmail.com","threadId":"31349","inReplyTo":"5039F327.9010003@gmail.com","subject":"Re: git no longer prompting for password","fromName":"Iain Paton","fromEmail":"ipaton0@gmail.com","sentAt":"2012-08-27T08:28:25Z","receivedAt":"2012-08-27T08:28:25Z","isPatch":false,"sender":{"key":"ipaton0@gmail.com","avatar":null},"body":"On 26/08/12 10:57, Iain Paton wrote:\n\n>         <If \"%{THE_REQUEST} =~ /git-receive-pack/\">\n\nI've just discovered that the <If ..> directive only appears in apache 2.4 \nso something more generic will probably be a better idea. Not everyone will \nbe running 2.4.x for a while yet.\n\nIain\n"},{"id":"197895","messageId":"20120827132145.GA17265@sigill.intra.peff.net","threadId":"31349","inReplyTo":"20120826101341.GA12566@sigill.intra.peff.net","subject":"[PATCH 0/8] fix password prompting for \"half-auth\" servers","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-08-27T13:21:45Z","receivedAt":"2012-08-27T13:21:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Aug 26, 2012 at 06:13:41AM -0400, Jeff King wrote:\n\n> No problem. I'll probably be a day or two on the patches, as the http\n> tests are in need of some refactoring before adding more tests. But in\n> the meantime, I think your config change is a sane work-around.\n\nOK, here is the series.  For those just joining us, the problem is that\ngit will not correctly prompt for credentials when pushing to a\nrepository which allows the initial GET of\n\".../info/refs?service=git-receive-pack\", but then gives a 401 when we\ntry to POST the pack. This has never worked for a plain URL, but used to\nwork if you put the username in the URL (because we would\nunconditionally load the credentials before making any requests). That\nwas broken by 986bbc0, which does not do that proactive prompting for\nsmart-http, meaning such repositories cannot be pushed to at all.\n\nSuch a server-side setup is questionable in my opinion (because the\nclient will actually create the pack before failing), but we have been\nadvertising it for a long time in git-http-backend(1) as the right way\nto make repositories that are anonymous for fetching but require auth\nfor pushing.\n\nThe fix is somewhat uglier than I would like, but I think it's practical\nand the right thing to do (see the final patch for lots of discussion).\nI built this on the current tip of \"master\".  It might make sense to\nbackport it directly on top of 986bbc0 for the maint track. There are\nconflicts, but they are all textual. Another option would be to revert\n986bbc0 for the maint track, as that commit is itself fixing a minor bug\nthat is of decreasing relevance (it fixed extra password prompting when\n.netrc was in use, but one can work around it by dropping the username\nfrom the URL).\n\nThe patches are:\n\n  [1/8]: t5550: put auth-required repo in auth/dumb\n  [2/8]: t5550: factor out http auth setup\n  [3/8]: t/lib-httpd: only route auth/dumb to dumb repos\n  [4/8]: t/lib-httpd: recognize */smart/* repos as smart-http\n  [5/8]: t: test basic smart-http authentication\n\nThese are all refactoring of the test scripts in preparation for 6/8\n(and are where all of the conflicts lie).\n\n  [6/8]: t: test http access to \"half-auth\" repositories\n\nThis demonstrates the bug.\n\n  [7/8]: http: factor out http error code handling\n\nRefactoring to support 8/8.\n\n  [8/8]: http: prompt for credentials on failed POST\n\nAnd this one is the actual fix.\n\nI'd like to have a 9/8 which tweaks the git-http-backend documentation\nto provide better example apache config, but I haven't yet figured out\nthe right incantation. Suggestions from apache gurus are welcome.\n\n-Peff\n"},{"id":"197896","messageId":"20120827132337.GA17375@sigill.intra.peff.net","threadId":"31349","inReplyTo":"20120827132145.GA17265@sigill.intra.peff.net","subject":"[PATCH 1/8] t5550: put auth-required repo in auth/dumb","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-08-27T13:23:37Z","receivedAt":"2012-08-27T13:23:37Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"In most of our tests, we put repos to be accessed by dumb\nprotocols in /dumb, and repos to be accessed by smart\nprotocols in /smart.  In our test apache setup, the whole\n/auth hierarchy requires authentication. However, we don't\nbother to split it by smart and dumb here because we are not\ncurrently testing smart-http authentication at all.\n\nThat will change in future patches, so let's be explicit\nthat we are interested in testing dumb access here. This\nalso happens to match what t5540 does for the push tests.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n t/t5550-http-fetch.sh | 18 +++++++++---------\n 1 file changed, 9 insertions(+), 9 deletions(-)\n\ndiff --git a/t/t5550-http-fetch.sh b/t/t5550-http-fetch.sh\nindex b06f817..5ad2123 100755\n--- a/t/t5550-http-fetch.sh\n+++ b/t/t5550-http-fetch.sh\n@@ -41,9 +41,9 @@ test_expect_success 'clone http repository' '\n '\n \n test_expect_success 'create password-protected repository' '\n-\tmkdir \"$HTTPD_DOCUMENT_ROOT_PATH/auth/\" &&\n+\tmkdir -p \"$HTTPD_DOCUMENT_ROOT_PATH/auth/dumb/\" &&\n \tcp -Rf \"$HTTPD_DOCUMENT_ROOT_PATH/repo.git\" \\\n-\t       \"$HTTPD_DOCUMENT_ROOT_PATH/auth/repo.git\"\n+\t       \"$HTTPD_DOCUMENT_ROOT_PATH/auth/dumb/repo.git\"\n '\n \n test_expect_success 'setup askpass helpers' '\n@@ -81,28 +81,28 @@ expect_askpass() {\n test_expect_success 'cloning password-protected repository can fail' '\n \t>askpass-query &&\n \techo wrong >askpass-response &&\n-\ttest_must_fail git clone \"$HTTPD_URL/auth/repo.git\" clone-auth-fail &&\n+\ttest_must_fail git clone \"$HTTPD_URL/auth/dumb/repo.git\" clone-auth-fail &&\n \texpect_askpass both wrong\n '\n \n test_expect_success 'http auth can use user/pass in URL' '\n \t>askpass-query &&\n \techo wrong >askpass-response &&\n-\tgit clone \"$HTTPD_URL_USER_PASS/auth/repo.git\" clone-auth-none &&\n+\tgit clone \"$HTTPD_URL_USER_PASS/auth/dumb/repo.git\" clone-auth-none &&\n \texpect_askpass none\n '\n \n test_expect_success 'http auth can use just user in URL' '\n \t>askpass-query &&\n \techo user@host >askpass-response &&\n-\tgit clone \"$HTTPD_URL_USER/auth/repo.git\" clone-auth-pass &&\n+\tgit clone \"$HTTPD_URL_USER/auth/dumb/repo.git\" clone-auth-pass &&\n \texpect_askpass pass user@host\n '\n \n test_expect_success 'http auth can request both user and pass' '\n \t>askpass-query &&\n \techo user@host >askpass-response &&\n-\tgit clone \"$HTTPD_URL/auth/repo.git\" clone-auth-both &&\n+\tgit clone \"$HTTPD_URL/auth/dumb/repo.git\" clone-auth-both &&\n \texpect_askpass both user@host\n '\n \n@@ -114,7 +114,7 @@ test_expect_success 'http auth respects credential helper config' '\n \t}; f\" &&\n \t>askpass-query &&\n \techo wrong >askpass-response &&\n-\tgit clone \"$HTTPD_URL/auth/repo.git\" clone-auth-helper &&\n+\tgit clone \"$HTTPD_URL/auth/dumb/repo.git\" clone-auth-helper &&\n \texpect_askpass none\n '\n \n@@ -122,7 +122,7 @@ test_expect_success 'http auth can get username from config' '\n \ttest_config_global \"credential.$HTTPD_URL.username\" user@host &&\n \t>askpass-query &&\n \techo user@host >askpass-response &&\n-\tgit clone \"$HTTPD_URL/auth/repo.git\" clone-auth-user &&\n+\tgit clone \"$HTTPD_URL/auth/dumb/repo.git\" clone-auth-user &&\n \texpect_askpass pass user@host\n '\n \n@@ -130,7 +130,7 @@ test_expect_success 'configured username does not override URL' '\n \ttest_config_global \"credential.$HTTPD_URL.username\" wrong &&\n \t>askpass-query &&\n \techo user@host >askpass-response &&\n-\tgit clone \"$HTTPD_URL_USER/auth/repo.git\" clone-auth-user2 &&\n+\tgit clone \"$HTTPD_URL_USER/auth/dumb/repo.git\" clone-auth-user2 &&\n \texpect_askpass pass user@host\n '\n \n-- \n1.7.11.5.10.g3c8125b\n"},{"id":"197897","messageId":"20120827132431.GB17375@sigill.intra.peff.net","threadId":"31349","inReplyTo":"20120827132145.GA17265@sigill.intra.peff.net","subject":"[PATCH 2/8] t5550: factor out http auth setup","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-08-27T13:24:31Z","receivedAt":"2012-08-27T13:24:31Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The t5550 script sets up a nice askpass helper for\nsimulating user input and checking what git prompted for.\nLet's make it available to other http scripts by migrating\nit to lib-httpd.\n\nWe can use this immediately in t5540 to make our tests more\nrobust (previously, we did not check at all that hitting the\npassword-protected repo actually involved a password).\nUnfortunately, we end up failing the test because the\ncurrent code erroneously prompts twice (once for\ngit-remote-http, and then again when the former spawns\ngit-http-push).\n\nMore importantly, though, it will let us easily add\nsmart-http authentication tests in t5541 and t5551; we\ncurrently do not test smart-http authentication at all.\n\nAs part of making it generic, let's always look for and\nstore auxiliary askpass files at the top-level trash\ndirectory; this makes it compatible with t5540, which runs\nsome tests from sub-repositories. We can abstract away the\nugliness with a short helper function.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nIf we do backport this to v1.7.8-era, note that write_script did not\nexist then.\n\n t/lib-httpd.sh        | 39 +++++++++++++++++++++++++++++++++++++\n t/t5540-http-push.sh  | 17 ++++++++---------\n t/t5550-http-fetch.sh | 53 ++++++++-------------------------------------------\n 3 files changed, 55 insertions(+), 54 deletions(-)\n\ndiff --git a/t/lib-httpd.sh b/t/lib-httpd.sh\nindex d773542..02f442b 100644\n--- a/t/lib-httpd.sh\n+++ b/t/lib-httpd.sh\n@@ -167,3 +167,42 @@ test_http_push_nonff() {\n \t\ttest_i18ngrep \"Updates were rejected because\" output\n \t'\n }\n+\n+setup_askpass_helper() {\n+\ttest_expect_success 'setup askpass helper' '\n+\t\twrite_script \"$TRASH_DIRECTORY/askpass\" <<-\\EOF &&\n+\t\techo >>\"$TRASH_DIRECTORY/askpass-query\" \"askpass: $*\" &&\n+\t\tcat \"$TRASH_DIRECTORY/askpass-response\"\n+\t\tEOF\n+\t\tGIT_ASKPASS=\"$TRASH_DIRECTORY/askpass\" &&\n+\t\texport GIT_ASKPASS &&\n+\t\texport TRASH_DIRECTORY\n+\t'\n+}\n+\n+set_askpass() {\n+\t>\"$TRASH_DIRECTORY/askpass-query\" &&\n+\techo \"$*\" >\"$TRASH_DIRECTORY/askpass-response\"\n+}\n+\n+expect_askpass() {\n+\tdest=$HTTPD_DEST\n+\t{\n+\t\tcase \"$1\" in\n+\t\tnone)\n+\t\t\t;;\n+\t\tpass)\n+\t\t\techo \"askpass: Password for 'http://$2@$dest': \"\n+\t\t\t;;\n+\t\tboth)\n+\t\t\techo \"askpass: Username for 'http://$dest': \"\n+\t\t\techo \"askpass: Password for 'http://$2@$dest': \"\n+\t\t\t;;\n+\t\t*)\n+\t\t\tfalse\n+\t\t\t;;\n+\t\tesac\n+\t} >\"$TRASH_DIRECTORY/askpass-expect\" &&\n+\ttest_cmp \"$TRASH_DIRECTORY/askpass-expect\" \\\n+\t\t \"$TRASH_DIRECTORY/askpass-query\"\n+}\ndiff --git a/t/t5540-http-push.sh b/t/t5540-http-push.sh\nindex 1eea647..f141f2d 100755\n--- a/t/t5540-http-push.sh\n+++ b/t/t5540-http-push.sh\n@@ -46,15 +46,7 @@ test_expect_success 'create password-protected repository' '\n \t       \"$HTTPD_DOCUMENT_ROOT_PATH/auth/dumb/test_repo.git\"\n '\n \n-test_expect_success 'setup askpass helper' '\n-\tcat >askpass <<-\\EOF &&\n-\t#!/bin/sh\n-\techo user@host\n-\tEOF\n-\tchmod +x askpass &&\n-\tGIT_ASKPASS=\"$PWD/askpass\" &&\n-\texport GIT_ASKPASS\n-'\n+setup_askpass_helper\n \n test_expect_success 'clone remote repository' '\n \tcd \"$ROOT_PATH\" &&\n@@ -162,6 +154,7 @@ test_http_push_nonff \"$HTTPD_DOCUMENT_ROOT_PATH\"/test_repo.git \\\n \n test_expect_success 'push to password-protected repository (user in URL)' '\n \ttest_commit pw-user &&\n+\tset_askpass user@host &&\n \tgit push \"$HTTPD_URL_USER/auth/dumb/test_repo.git\" HEAD &&\n \tgit rev-parse --verify HEAD >expect &&\n \tgit --git-dir=\"$HTTPD_DOCUMENT_ROOT_PATH/auth/dumb/test_repo.git\" \\\n@@ -169,9 +162,15 @@ test_expect_success 'push to password-protected repository (user in URL)' '\n \ttest_cmp expect actual\n '\n \n+test_expect_failure 'user was prompted only once for password' '\n+\texpect_askpass pass user@host\n+'\n+\n test_expect_failure 'push to password-protected repository (no user in URL)' '\n \ttest_commit pw-nouser &&\n+\tset_askpass user@host &&\n \tgit push \"$HTTPD_URL/auth/dumb/test_repo.git\" HEAD &&\n+\texpect_askpass both user@host\n \tgit rev-parse --verify HEAD >expect &&\n \tgit --git-dir=\"$HTTPD_DOCUMENT_ROOT_PATH/auth/dumb/test_repo.git\" \\\n \t\trev-parse --verify HEAD >actual &&\ndiff --git a/t/t5550-http-fetch.sh b/t/t5550-http-fetch.sh\nindex 5ad2123..16ef041 100755\n--- a/t/t5550-http-fetch.sh\n+++ b/t/t5550-http-fetch.sh\n@@ -46,62 +46,28 @@ test_expect_success 'create password-protected repository' '\n \t       \"$HTTPD_DOCUMENT_ROOT_PATH/auth/dumb/repo.git\"\n '\n \n-test_expect_success 'setup askpass helpers' '\n-\tcat >askpass <<-EOF &&\n-\t#!/bin/sh\n-\techo >>\"$PWD/askpass-query\" \"askpass: \\$*\" &&\n-\tcat \"$PWD/askpass-response\"\n-\tEOF\n-\tchmod +x askpass &&\n-\tGIT_ASKPASS=\"$PWD/askpass\" &&\n-\texport GIT_ASKPASS\n-'\n-\n-expect_askpass() {\n-\tdest=$HTTPD_DEST\n-\t{\n-\t\tcase \"$1\" in\n-\t\tnone)\n-\t\t\t;;\n-\t\tpass)\n-\t\t\techo \"askpass: Password for 'http://$2@$dest': \"\n-\t\t\t;;\n-\t\tboth)\n-\t\t\techo \"askpass: Username for 'http://$dest': \"\n-\t\t\techo \"askpass: Password for 'http://$2@$dest': \"\n-\t\t\t;;\n-\t\t*)\n-\t\t\tfalse\n-\t\t\t;;\n-\t\tesac\n-\t} >askpass-expect &&\n-\ttest_cmp askpass-expect askpass-query\n-}\n+setup_askpass_helper\n \n test_expect_success 'cloning password-protected repository can fail' '\n-\t>askpass-query &&\n-\techo wrong >askpass-response &&\n+\tset_askpass wrong &&\n \ttest_must_fail git clone \"$HTTPD_URL/auth/dumb/repo.git\" clone-auth-fail &&\n \texpect_askpass both wrong\n '\n \n test_expect_success 'http auth can use user/pass in URL' '\n-\t>askpass-query &&\n-\techo wrong >askpass-response &&\n+\tset_askpass wrong &&\n \tgit clone \"$HTTPD_URL_USER_PASS/auth/dumb/repo.git\" clone-auth-none &&\n \texpect_askpass none\n '\n \n test_expect_success 'http auth can use just user in URL' '\n-\t>askpass-query &&\n-\techo user@host >askpass-response &&\n+\tset_askpass user@host &&\n \tgit clone \"$HTTPD_URL_USER/auth/dumb/repo.git\" clone-auth-pass &&\n \texpect_askpass pass user@host\n '\n \n test_expect_success 'http auth can request both user and pass' '\n-\t>askpass-query &&\n-\techo user@host >askpass-response &&\n+\tset_askpass user@host &&\n \tgit clone \"$HTTPD_URL/auth/dumb/repo.git\" clone-auth-both &&\n \texpect_askpass both user@host\n '\n@@ -112,24 +78,21 @@ test_expect_success 'http auth respects credential helper config' '\n \t\techo username=user@host\n \t\techo password=user@host\n \t}; f\" &&\n-\t>askpass-query &&\n-\techo wrong >askpass-response &&\n+\tset_askpass wrong &&\n \tgit clone \"$HTTPD_URL/auth/dumb/repo.git\" clone-auth-helper &&\n \texpect_askpass none\n '\n \n test_expect_success 'http auth can get username from config' '\n \ttest_config_global \"credential.$HTTPD_URL.username\" user@host &&\n-\t>askpass-query &&\n-\techo user@host >askpass-response &&\n+\tset_askpass user@host &&\n \tgit clone \"$HTTPD_URL/auth/dumb/repo.git\" clone-auth-user &&\n \texpect_askpass pass user@host\n '\n \n test_expect_success 'configured username does not override URL' '\n \ttest_config_global \"credential.$HTTPD_URL.username\" wrong &&\n-\t>askpass-query &&\n-\techo user@host >askpass-response &&\n+\tset_askpass user@host &&\n \tgit clone \"$HTTPD_URL_USER/auth/dumb/repo.git\" clone-auth-user2 &&\n \texpect_askpass pass user@host\n '\n-- \n1.7.11.5.10.g3c8125b\n"},{"id":"197898","messageId":"20120827132442.GC17375@sigill.intra.peff.net","threadId":"31349","inReplyTo":"20120827132145.GA17265@sigill.intra.peff.net","subject":"[PATCH 3/8] t/lib-httpd: only route auth/dumb to dumb repos","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-08-27T13:24:42Z","receivedAt":"2012-08-27T13:24:42Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Our test apache config points all of auth/ directly to the\non-disk repositories via an Alias directive. This works fine\nbecause everything authenticated is currently in auth/dumb,\nwhich is a subset.  However, this would conflict with a\nScriptAlias for auth/smart (which will come in future\npatches), so let's narrow the Alias.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n t/lib-httpd/apache.conf | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/lib-httpd/apache.conf b/t/lib-httpd/apache.conf\nindex 36b1596..d13fe64 100644\n--- a/t/lib-httpd/apache.conf\n+++ b/t/lib-httpd/apache.conf\n@@ -46,7 +46,7 @@ PassEnv GIT_VALGRIND\n PassEnv GIT_VALGRIND_OPTIONS\n \n Alias /dumb/ www/\n-Alias /auth/ www/auth/\n+Alias /auth/dumb/ www/auth/dumb/\n \n <Location /smart/>\n \tSetEnv GIT_EXEC_PATH ${GIT_EXEC_PATH}\n-- \n1.7.11.5.10.g3c8125b\n"},{"id":"197899","messageId":"20120827132521.GD17375@sigill.intra.peff.net","threadId":"31349","inReplyTo":"20120827132145.GA17265@sigill.intra.peff.net","subject":"[PATCH 4/8] t/lib-httpd: recognize */smart/* repos as smart-http","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-08-27T13:25:21Z","receivedAt":"2012-08-27T13:25:21Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We do not currently test authentication for smart-http repos\nat all. Part of the infrastructure to do this is recognizing\nthat auth/smart is indeed a smart-http repo.\n\nThe current apache config recognizes only \"^/smart/*\" as\nsmart-http. Let's instead treat anything with /smart/ in the\nURL as smart-http. This is obviously a stupid thing to do\nfor a real production site, but for our test suite we know\nthat our repositories will not have this magic string in the\nname.\n\nNote that we will route /foo/smart/bar.git directly to\ngit-http-backend/bar.git; in other words, everything before\nthe \"/smart/\" is irrelevant to finding the repo on disk (but\nmay impact apache config, for example by triggering auth\nchecks).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nAnother backporting gotcha: the smart_custom_env bits did not exist back\nin the v1.7.8 era.\n\n t/lib-httpd/apache.conf | 16 +++++++---------\n 1 file changed, 7 insertions(+), 9 deletions(-)\n\ndiff --git a/t/lib-httpd/apache.conf b/t/lib-httpd/apache.conf\nindex d13fe64..c6a1a87 100644\n--- a/t/lib-httpd/apache.conf\n+++ b/t/lib-httpd/apache.conf\n@@ -48,22 +48,20 @@ PassEnv GIT_VALGRIND_OPTIONS\n Alias /dumb/ www/\n Alias /auth/dumb/ www/auth/dumb/\n \n-<Location /smart/>\n+<LocationMatch /smart/>\n \tSetEnv GIT_EXEC_PATH ${GIT_EXEC_PATH}\n \tSetEnv GIT_HTTP_EXPORT_ALL\n-</Location>\n-<Location /smart_noexport/>\n+</LocationMatch>\n+<LocationMatch /smart_noexport/>\n \tSetEnv GIT_EXEC_PATH ${GIT_EXEC_PATH}\n-</Location>\n-<Location /smart_custom_env/>\n+</LocationMatch>\n+<LocationMatch /smart_custom_env/>\n \tSetEnv GIT_EXEC_PATH ${GIT_EXEC_PATH}\n \tSetEnv GIT_HTTP_EXPORT_ALL\n \tSetEnv GIT_COMMITTER_NAME \"Custom User\"\n \tSetEnv GIT_COMMITTER_EMAIL custom@example.com\n-</Location>\n-ScriptAlias /smart/ ${GIT_EXEC_PATH}/git-http-backend/\n-ScriptAlias /smart_noexport/ ${GIT_EXEC_PATH}/git-http-backend/\n-ScriptAlias /smart_custom_env/ ${GIT_EXEC_PATH}/git-http-backend/\n+</LocationMatch>\n+ScriptAliasMatch /smart_*[^/]*/(.*) ${GIT_EXEC_PATH}/git-http-backend/$1\n <Directory ${GIT_EXEC_PATH}>\n \tOptions FollowSymlinks\n </Directory>\n-- \n1.7.11.5.10.g3c8125b\n"},{"id":"197900","messageId":"20120827132536.GE17375@sigill.intra.peff.net","threadId":"31349","inReplyTo":"20120827132145.GA17265@sigill.intra.peff.net","subject":"[PATCH 5/8] t: test basic smart-http authentication","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-08-27T13:25:36Z","receivedAt":"2012-08-27T13:25:36Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We do not currently test authentication over smart-http at\nall. In theory, it should work exactly as it does for dumb\nhttp (which we do test). It does indeed work for these\nsimple tests, but this patch lays the groundwork for more\ncomplex tests in future patches.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n t/t5541-http-push.sh  | 14 ++++++++++++++\n t/t5551-http-fetch.sh | 11 +++++++++++\n 2 files changed, 25 insertions(+)\n\ndiff --git a/t/t5541-http-push.sh b/t/t5541-http-push.sh\nindex 312e484..eeb9932 100755\n--- a/t/t5541-http-push.sh\n+++ b/t/t5541-http-push.sh\n@@ -36,6 +36,8 @@ test_expect_success 'setup remote repository' '\n \tmv test_repo.git \"$HTTPD_DOCUMENT_ROOT_PATH\"\n '\n \n+setup_askpass_helper\n+\n cat >exp <<EOF\n GET  /smart/test_repo.git/info/refs?service=git-upload-pack HTTP/1.1 200\n POST /smart/test_repo.git/git-upload-pack HTTP/1.1 200\n@@ -266,5 +268,17 @@ test_expect_success 'http push respects GIT_COMMITTER_* in reflog' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'push over smart http with auth' '\n+\tcd \"$ROOT_PATH/test_repo_clone\" &&\n+\techo push-auth-test >expect &&\n+\ttest_commit push-auth-test &&\n+\tset_askpass user@host &&\n+\tgit push \"$HTTPD_URL\"/auth/smart/test_repo.git &&\n+\tgit --git-dir=\"$HTTPD_DOCUMENT_ROOT_PATH/test_repo.git\" \\\n+\t\tlog -1 --format=%s >actual &&\n+\texpect_askpass both user@host &&\n+\ttest_cmp expect actual\n+'\n+\n stop_httpd\n test_done\ndiff --git a/t/t5551-http-fetch.sh b/t/t5551-http-fetch.sh\nindex 91eaf53..e653ae3 100755\n--- a/t/t5551-http-fetch.sh\n+++ b/t/t5551-http-fetch.sh\n@@ -27,6 +27,8 @@ test_expect_success 'create http-accessible bare repository' '\n \tgit push public master:master\n '\n \n+setup_askpass_helper\n+\n cat >exp <<EOF\n > GET /smart/repo.git/info/refs?service=git-upload-pack HTTP/1.1\n > Accept: */*\n@@ -109,6 +111,15 @@ test_expect_success 'follow redirects (302)' '\n \tgit clone $HTTPD_URL/smart-redir-temp/repo.git --quiet repo-t\n '\n \n+test_expect_success 'clone from password-protected repository' '\n+\techo two >expect &&\n+\tset_askpass user@host &&\n+\tgit clone --bare \"$HTTPD_URL/auth/smart/repo.git\" smart-auth &&\n+\texpect_askpass both user@host &&\n+\tgit --git-dir=smart-auth log -1 --format=%s >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test -n \"$GIT_TEST_LONG\" && test_set_prereq EXPENSIVE\n \n test_expect_success EXPENSIVE 'create 50,000 tags in the repo' '\n-- \n1.7.11.5.10.g3c8125b\n"},{"id":"197901","messageId":"20120827132553.GF17375@sigill.intra.peff.net","threadId":"31349","inReplyTo":"20120827132145.GA17265@sigill.intra.peff.net","subject":"[PATCH 6/8] t: test http access to \"half-auth\" repositories","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-08-27T13:25:53Z","receivedAt":"2012-08-27T13:25:53Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Some sites set up http access to repositories such that\nfetching is anonymous and unauthenticated, but pushing is\nauthenticated. While there are multiple ways to do this, the\ntechnique advertised in the git-http-backend manpage is to\nblock access to locations matching \"/git-receive-pack$\".\n\nLet's emulate that advice in our test setup, which makes it\nclear that this advice does not actually work.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n t/lib-httpd/apache.conf |  7 +++++++\n t/t5541-http-push.sh    | 12 ++++++++++++\n t/t5551-http-fetch.sh   |  9 +++++++++\n 3 files changed, 28 insertions(+)\n\ndiff --git a/t/lib-httpd/apache.conf b/t/lib-httpd/apache.conf\nindex c6a1a87..49d5d87 100644\n--- a/t/lib-httpd/apache.conf\n+++ b/t/lib-httpd/apache.conf\n@@ -92,6 +92,13 @@ SSLEngine On\n \tRequire valid-user\n </Location>\n \n+<LocationMatch \"^/auth-push/.*/git-receive-pack$\">\n+\tAuthType Basic\n+\tAuthName \"git-auth\"\n+\tAuthUserFile passwd\n+\tRequire valid-user\n+</LocationMatch>\n+\n <IfDefine DAV>\n \tLoadModule dav_module modules/mod_dav.so\n \tLoadModule dav_fs_module modules/mod_dav_fs.so\ndiff --git a/t/t5541-http-push.sh b/t/t5541-http-push.sh\nindex eeb9932..9b1cd60 100755\n--- a/t/t5541-http-push.sh\n+++ b/t/t5541-http-push.sh\n@@ -280,5 +280,17 @@ test_expect_success 'push over smart http with auth' '\n \ttest_cmp expect actual\n '\n \n+test_expect_failure 'push to auth-only-for-push repo' '\n+\tcd \"$ROOT_PATH/test_repo_clone\" &&\n+\techo push-half-auth >expect &&\n+\ttest_commit push-half-auth &&\n+\tset_askpass user@host &&\n+\tgit push \"$HTTPD_URL\"/auth-push/smart/test_repo.git &&\n+\tgit --git-dir=\"$HTTPD_DOCUMENT_ROOT_PATH/test_repo.git\" \\\n+\t\tlog -1 --format=%s >actual &&\n+\texpect_askpass both user@host &&\n+\ttest_cmp expect actual\n+'\n+\n stop_httpd\n test_done\ndiff --git a/t/t5551-http-fetch.sh b/t/t5551-http-fetch.sh\nindex e653ae3..2db5c35 100755\n--- a/t/t5551-http-fetch.sh\n+++ b/t/t5551-http-fetch.sh\n@@ -120,6 +120,15 @@ test_expect_success 'clone from password-protected repository' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'clone from auth-only-for-push repository' '\n+\techo two >expect &&\n+\tset_askpass wrong &&\n+\tgit clone --bare \"$HTTPD_URL/auth-push/smart/repo.git\" smart-noauth &&\n+\texpect_askpass none &&\n+\tgit --git-dir=smart-noauth log -1 --format=%s >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test -n \"$GIT_TEST_LONG\" && test_set_prereq EXPENSIVE\n \n test_expect_success EXPENSIVE 'create 50,000 tags in the repo' '\n-- \n1.7.11.5.10.g3c8125b\n"},{"id":"197902","messageId":"20120827132604.GG17375@sigill.intra.peff.net","threadId":"31349","inReplyTo":"20120827132145.GA17265@sigill.intra.peff.net","subject":"[PATCH 7/8] http: factor out http error code handling","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-08-27T13:26:04Z","receivedAt":"2012-08-27T13:26:04Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Most of our http requests go through the http_request()\ninterface, which does some nice post-processing on the\nresults. In particular, it handles prompting for missing\ncredentials as well as approving and rejecting valid or\ninvalid credentials. Unfortunately, it only handles GET\nrequests. Making it handle POSTs would be quite complex, so\nlet's pull result handling code into its own function so\nthat it can be reused from the POST code paths.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n http.c | 51 ++++++++++++++++++++++++++++-----------------------\n http.h |  1 +\n 2 files changed, 29 insertions(+), 23 deletions(-)\n\ndiff --git a/http.c b/http.c\nindex b61ac85..6793137 100644\n--- a/http.c\n+++ b/http.c\n@@ -745,6 +745,33 @@ char *get_remote_object_url(const char *url, const char *hex,\n \treturn strbuf_detach(&buf, NULL);\n }\n \n+int handle_curl_result(struct active_request_slot *slot)\n+{\n+\tstruct slot_results *results = slot->results;\n+\n+\tif (results->curl_result == CURLE_OK) {\n+\t\tcredential_approve(&http_auth);\n+\t\treturn HTTP_OK;\n+\t} else if (missing_target(results))\n+\t\treturn HTTP_MISSING_TARGET;\n+\telse if (results->http_code == 401) {\n+\t\tif (http_auth.username && http_auth.password) {\n+\t\t\tcredential_reject(&http_auth);\n+\t\t\treturn HTTP_NOAUTH;\n+\t\t} else {\n+\t\t\tcredential_fill(&http_auth);\n+\t\t\tinit_curl_http_auth(slot->curl);\n+\t\t\treturn HTTP_REAUTH;\n+\t\t}\n+\t} else {\n+\t\tif (!curl_errorstr[0])\n+\t\t\tstrlcpy(curl_errorstr,\n+\t\t\t\tcurl_easy_strerror(results->curl_result),\n+\t\t\t\tsizeof(curl_errorstr));\n+\t\treturn HTTP_ERROR;\n+\t}\n+}\n+\n /* http_request() targets */\n #define HTTP_REQUEST_STRBUF\t0\n #define HTTP_REQUEST_FILE\t1\n@@ -792,26 +819,7 @@ static int http_request(const char *url, void *result, int target, int options)\n \n \tif (start_active_slot(slot)) {\n \t\trun_active_slot(slot);\n-\t\tif (results.curl_result == CURLE_OK)\n-\t\t\tret = HTTP_OK;\n-\t\telse if (missing_target(&results))\n-\t\t\tret = HTTP_MISSING_TARGET;\n-\t\telse if (results.http_code == 401) {\n-\t\t\tif (http_auth.username && http_auth.password) {\n-\t\t\t\tcredential_reject(&http_auth);\n-\t\t\t\tret = HTTP_NOAUTH;\n-\t\t\t} else {\n-\t\t\t\tcredential_fill(&http_auth);\n-\t\t\t\tinit_curl_http_auth(slot->curl);\n-\t\t\t\tret = HTTP_REAUTH;\n-\t\t\t}\n-\t\t} else {\n-\t\t\tif (!curl_errorstr[0])\n-\t\t\t\tstrlcpy(curl_errorstr,\n-\t\t\t\t\tcurl_easy_strerror(results.curl_result),\n-\t\t\t\t\tsizeof(curl_errorstr));\n-\t\t\tret = HTTP_ERROR;\n-\t\t}\n+\t\tret = handle_curl_result(slot);\n \t} else {\n \t\terror(\"Unable to start HTTP request for %s\", url);\n \t\tret = HTTP_START_FAILED;\n@@ -820,9 +828,6 @@ static int http_request(const char *url, void *result, int target, int options)\n \tcurl_slist_free_all(headers);\n \tstrbuf_release(&buf);\n \n-\tif (ret == HTTP_OK)\n-\t\tcredential_approve(&http_auth);\n-\n \treturn ret;\n }\n \ndiff --git a/http.h b/http.h\nindex 915c286..12de255 100644\n--- a/http.h\n+++ b/http.h\n@@ -78,6 +78,7 @@ extern int start_active_slot(struct active_request_slot *slot);\n extern void run_active_slot(struct active_request_slot *slot);\n extern void finish_active_slot(struct active_request_slot *slot);\n extern void finish_all_active_slots(void);\n+extern int handle_curl_result(struct active_request_slot *slot);\n \n #ifdef USE_CURL_MULTI\n extern void fill_active_slots(void);\n-- \n1.7.11.5.10.g3c8125b\n"},{"id":"197903","messageId":"20120827132714.GH17375@sigill.intra.peff.net","threadId":"31349","inReplyTo":"20120827132145.GA17265@sigill.intra.peff.net","subject":"[PATCH 8/8] http: prompt for credentials on failed POST","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-08-27T13:27:15Z","receivedAt":"2012-08-27T13:27:15Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"All of the smart-http GET requests go through the http_get_*\nfunctions, which will prompt for credentials and retry if we\nsee an HTTP 401.\n\nPOST requests, however, do not go through any central point.\nMoreover, it is difficult to retry in the general case; we\ncannot assume the request body fits in memory or is even\nseekable, and we don't know how much of it was consumed\nduring the attempt.\n\nMost of the time, this is not a big deal; for both fetching\nand pushing, we make a GET request before doing any POSTs,\nso typically we figure out the credentials during the first\nrequest, then reuse them during the POST. However, some\nservers may allow a client to get the list of refs from\nreceive-pack without authentication, and then require\nauthentication when the client actually tries to POST the\npack.\n\nThis is not ideal, as the client may do a non-trivial amount\nof work to generate the pack (e.g., delta-compressing\nobjects). However, for a long time it has been the\nrecommended example configuration in git-http-backend(1) for\nsetting up a repository with anonymous fetch and\nauthenticated push. This setup has always been broken\nwithout putting a username into the URL. Prior to commit\n986bbc0, it did work with a username in the URL, because git\nwould prompt for credentials before making any requests at\nall. However, post-986bbc0, it is totally broken. Since it\nhas been advertised in the manpage for some time, we should\nmake sure it works.\n\nUnfortunately, it is not as easy as simply calling post_rpc\nagain when it fails, due to the input issue mentioned above.\nHowever, we can still make this specific case work by\nretrying in two specific instances:\n\n  1. If the request is large (bigger than LARGE_PACKET_MAX),\n     we will first send a probe request with a single flush\n     packet. Since this request is static, we can freely\n     retry it.\n\n  2. If the request is small and we are not using gzip, then\n     we have the whole thing in-core, and we can freely\n     retry.\n\nThat means we will not retry in some instances, including:\n\n  1. If we are using gzip. However, we only do so when\n     calling git-upload-pack, so it does not apply to\n     pushes.\n\n  2. If we have a large request, the probe succeeds, but\n     then the real POST wants authentication. This is an\n     extremely unlikely configuration and not worth worrying\n     about.\n\nWhile it might be nice to cover those instances, doing so\nwould be significantly more complex for very little\nreal-world gain. In the long run, we will be much better off\nwhen curl learns to internally handle authentication as a\ncallback, and we can cleanly handle all cases that way.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nSorry for the wordy explanation. I really tried to refactor this into a\nnice single code path for making both GET and POST requests, but I think\nthere are just too many corner cases. Suggestions welcome if somebody\nhas a better idea of how to refactor it (preferably in the form of a\npatch).\n\n remote-curl.c        | 23 +++++++++++++++--------\n t/t5541-http-push.sh |  2 +-\n 2 files changed, 16 insertions(+), 9 deletions(-)\n\ndiff --git a/remote-curl.c b/remote-curl.c\nindex 04a9d62..3ec474f 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -362,16 +362,17 @@ static size_t rpc_in(char *ptr, size_t eltsize,\n \n static int run_slot(struct active_request_slot *slot)\n {\n-\tint err = 0;\n+\tint err;\n \tstruct slot_results results;\n \n \tslot->results = &results;\n \tslot->curl_result = curl_easy_perform(slot->curl);\n \tfinish_active_slot(slot);\n \n-\tif (results.curl_result != CURLE_OK) {\n-\t\terr |= error(\"RPC failed; result=%d, HTTP code = %ld\",\n-\t\t\tresults.curl_result, results.http_code);\n+\terr = handle_curl_result(slot);\n+\tif (err != HTTP_OK && err != HTTP_REAUTH) {\n+\t\terror(\"RPC failed; result=%d, HTTP code = %ld\",\n+\t\t      results.curl_result, results.http_code);\n \t}\n \n \treturn err;\n@@ -436,9 +437,11 @@ static int post_rpc(struct rpc_state *rpc)\n \t}\n \n \tif (large_request) {\n-\t\terr = probe_rpc(rpc);\n-\t\tif (err)\n-\t\t\treturn err;\n+\t\tdo {\n+\t\t\terr = probe_rpc(rpc);\n+\t\t} while (err == HTTP_REAUTH);\n+\t\tif (err != HTTP_OK)\n+\t\t\treturn -1;\n \t}\n \n \tslot = get_active_slot();\n@@ -525,7 +528,11 @@ static int post_rpc(struct rpc_state *rpc)\n \tcurl_easy_setopt(slot->curl, CURLOPT_WRITEFUNCTION, rpc_in);\n \tcurl_easy_setopt(slot->curl, CURLOPT_FILE, rpc);\n \n-\terr = run_slot(slot);\n+\tdo {\n+\t\terr = run_slot(slot);\n+\t} while (err == HTTP_REAUTH && !large_request && !use_gzip);\n+\tif (err != HTTP_OK)\n+\t\terr = -1;\n \n \tcurl_slist_free_all(headers);\n \tfree(gzip_body);\ndiff --git a/t/t5541-http-push.sh b/t/t5541-http-push.sh\nindex 9b1cd60..ef6d6b6 100755\n--- a/t/t5541-http-push.sh\n+++ b/t/t5541-http-push.sh\n@@ -280,7 +280,7 @@ test_expect_success 'push over smart http with auth' '\n \ttest_cmp expect actual\n '\n \n-test_expect_failure 'push to auth-only-for-push repo' '\n+test_expect_success 'push to auth-only-for-push repo' '\n \tcd \"$ROOT_PATH/test_repo_clone\" &&\n \techo push-half-auth >expect &&\n \ttest_commit push-half-auth &&\n-- \n1.7.11.5.10.g3c8125b\n"},{"id":"197904","messageId":"BE91037D-8D8A-436C-BA21-6D18CB9BC87B@bjhargrave.com","threadId":"31349","inReplyTo":"503B2FA9.6030700@gmail.com","subject":"Re: git no longer prompting for password","fromName":"BJ Hargrave","fromEmail":"bj@bjhargrave.com","sentAt":"2012-08-27T13:33:42Z","receivedAt":"2012-08-27T13:33:42Z","isPatch":false,"sender":{"key":"bj@bjhargrave.com","avatar":"https://gravatar.com/avatar/48e60c01177c0e8d3e60c996d54fbe36cf70058efcdd020375b4055e34fc05d7?d=mp&s=160"},"body":"On Aug 27, 2012, at 04:28 , Iain Paton wrote:\n\n> On 26/08/12 10:57, Iain Paton wrote:\n> \n>>        <If \"%{THE_REQUEST} =~ /git-receive-pack/\">\n> \n> I've just discovered that the <If ..> directive only appears in apache 2.4 \n> so something more generic will probably be a better idea. Not everyone will \n> be running 2.4.x for a while yet.\n\n\nYou could try something like this:\n\n<Location /git>\n  # Require authentication for git push\n  RewriteCond %{QUERY_STRING} service=git-receive-pack\n  RewriteRule .* - [E=AUTHREQUIRED:yes]\n  Order Allow,Deny\n  Deny from env=AUTHREQUIRED\n  Allow from all\n  Satisfy Any\n  # Whatever auth rules you want ...\n\nI 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.\n-- \n\nBJ\n"},{"id":"197921","messageId":"7vbohws1dw.fsf@alter.siamese.dyndns.org","threadId":"31349","inReplyTo":"20120827132145.GA17265@sigill.intra.peff.net","subject":"Re: [PATCH 0/8] fix password prompting for \"half-auth\" servers","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-08-27T17:14:35Z","receivedAt":"2012-08-27T17:14:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n(+cc: Shawn)\n\n> On Sun, Aug 26, 2012 at 06:13:41AM -0400, Jeff King wrote:\n>\n>> No problem. I'll probably be a day or two on the patches, as the http\n>> tests are in need of some refactoring before adding more tests. But in\n>> the meantime, I think your config change is a sane work-around.\n>\n> OK, here is the series.  For those just joining us, the problem is that\n> git will not correctly prompt for credentials when pushing to a\n> repository which allows the initial GET of\n> \".../info/refs?service=git-receive-pack\", but then gives a 401 when we\n> try to POST the pack. This has never worked for a plain URL, but used to\n> work if you put the username in the URL (because we would\n> unconditionally load the credentials before making any requests). That\n> was broken by 986bbc0, which does not do that proactive prompting for\n> smart-http, meaning such repositories cannot be pushed to at all.\n>\n> Such a server-side setup is questionable in my opinion (because the\n> client will actually create the pack before failing), but we have been\n> advertising it for a long time in git-http-backend(1) as the right way\n> to make repositories that are anonymous for fetching but require auth\n> for pushing.\n>\n> The fix is somewhat uglier than I would like, but I think it's practical\n> and the right thing to do (see the final patch for lots of discussion).\n> I built this on the current tip of \"master\".  It might make sense to\n> backport it directly on top of 986bbc0 for the maint track. There are\n> conflicts, but they are all textual. Another option would be to revert\n> 986bbc0 for the maint track, as that commit is itself fixing a minor bug\n> that is of decreasing relevance (it fixed extra password prompting when\n> .netrc was in use, but one can work around it by dropping the username\n> from the URL).\n>\n> The patches are:\n>\n>   [1/8]: t5550: put auth-required repo in auth/dumb\n>   [2/8]: t5550: factor out http auth setup\n>   [3/8]: t/lib-httpd: only route auth/dumb to dumb repos\n>   [4/8]: t/lib-httpd: recognize */smart/* repos as smart-http\n>   [5/8]: t: test basic smart-http authentication\n>\n> These are all refactoring of the test scripts in preparation for 6/8\n> (and are where all of the conflicts lie).\n>\n>   [6/8]: t: test http access to \"half-auth\" repositories\n>\n> This demonstrates the bug.\n>\n>   [7/8]: http: factor out http error code handling\n>\n> Refactoring to support 8/8.\n>\n>   [8/8]: http: prompt for credentials on failed POST\n>\n> And this one is the actual fix.\n>\n> I'd like to have a 9/8 which tweaks the git-http-backend documentation\n> to provide better example apache config, but I haven't yet figured out\n> the right incantation. Suggestions from apache gurus are welcome.\n>\n> -Peff\n"},{"id":"197923","messageId":"7v3938rztf.fsf@alter.siamese.dyndns.org","threadId":"31349","inReplyTo":"20120827132714.GH17375@sigill.intra.peff.net","subject":"Re: [PATCH 8/8] http: prompt for credentials on failed POST","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-08-27T17:48:28Z","receivedAt":"2012-08-27T17:48:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> Most of the time, this is not a big deal; for both fetching\n> and pushing, we make a GET request before doing any POSTs,\n> so typically we figure out the credentials during the first\n> request, then reuse them during the POST. However, some\n> servers may allow a client to get the list of refs from\n> receive-pack without authentication, and then require\n> authentication when the client actually tries to POST the\n> pack.\n\nA silly question.  Does the initial GET request when we push look\nany different from the initial GET request when we fetch?  Can we\nmake them look different in an updated client, so that the server\nside can say \"this GET is about pushing into us, and we require\nauthentication\"?\n\n> Unfortunately, it is not as easy as simply calling post_rpc\n> again when it fails, due to the input issue mentioned above.\n> However, we can still make this specific case work by\n> retrying in two specific instances:\n>\n>   1. If the request is large (bigger than LARGE_PACKET_MAX),\n>      we will first send a probe request with a single flush\n>      packet. Since this request is static, we can freely\n>      retry it.\n>\n>   2. If the request is small and we are not using gzip, then\n>      we have the whole thing in-core, and we can freely\n>      retry.\n>\n> That means we will not retry in some instances, including:\n>\n>   1. If we are using gzip. However, we only do so when\n>      calling git-upload-pack, so it does not apply to\n>      pushes.\n>\n>   2. If we have a large request, the probe succeeds, but\n>      then the real POST wants authentication. This is an\n>      extremely unlikely configuration and not worth worrying\n>      about.\n>\n> While it might be nice to cover those instances, doing so\n> would be significantly more complex for very little\n> real-world gain. In the long run, we will be much better off\n> when curl learns to internally handle authentication as a\n> callback, and we can cleanly handle all cases that way.\n\nI suspect that in real life, almost nobody runs smart HTTP server\nthat allows anonymous push.\n\nHow much usability penalty would it be if we always fill credential\nbefore pushing?  Alternatively, how much latency penalty would it\nincur if we always send a probe request regardless of the request\nsize when we try to push without having an authentication material?\n\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n> Sorry for the wordy explanation. I really tried to refactor this into a\n> nice single code path for making both GET and POST requests, but I think\n> there are just too many corner cases. Suggestions welcome if somebody\n> has a better idea of how to refactor it (preferably in the form of a\n> patch).\n>\n>  remote-curl.c        | 23 +++++++++++++++--------\n>  t/t5541-http-push.sh |  2 +-\n>  2 files changed, 16 insertions(+), 9 deletions(-)\n>\n> diff --git a/remote-curl.c b/remote-curl.c\n> index 04a9d62..3ec474f 100644\n> --- a/remote-curl.c\n> +++ b/remote-curl.c\n> @@ -362,16 +362,17 @@ static size_t rpc_in(char *ptr, size_t eltsize,\n>  \n>  static int run_slot(struct active_request_slot *slot)\n>  {\n> -\tint err = 0;\n> +\tint err;\n>  \tstruct slot_results results;\n>  \n>  \tslot->results = &results;\n>  \tslot->curl_result = curl_easy_perform(slot->curl);\n>  \tfinish_active_slot(slot);\n>  \n> -\tif (results.curl_result != CURLE_OK) {\n> -\t\terr |= error(\"RPC failed; result=%d, HTTP code = %ld\",\n> -\t\t\tresults.curl_result, results.http_code);\n> +\terr = handle_curl_result(slot);\n> +\tif (err != HTTP_OK && err != HTTP_REAUTH) {\n> +\t\terror(\"RPC failed; result=%d, HTTP code = %ld\",\n> +\t\t      results.curl_result, results.http_code);\n>  \t}\n>  \n>  \treturn err;\n> @@ -436,9 +437,11 @@ static int post_rpc(struct rpc_state *rpc)\n>  \t}\n>  \n>  \tif (large_request) {\n> -\t\terr = probe_rpc(rpc);\n> -\t\tif (err)\n> -\t\t\treturn err;\n> +\t\tdo {\n> +\t\t\terr = probe_rpc(rpc);\n> +\t\t} while (err == HTTP_REAUTH);\n> +\t\tif (err != HTTP_OK)\n> +\t\t\treturn -1;\n>  \t}\n>  \n>  \tslot = get_active_slot();\n> @@ -525,7 +528,11 @@ static int post_rpc(struct rpc_state *rpc)\n>  \tcurl_easy_setopt(slot->curl, CURLOPT_WRITEFUNCTION, rpc_in);\n>  \tcurl_easy_setopt(slot->curl, CURLOPT_FILE, rpc);\n>  \n> -\terr = run_slot(slot);\n> +\tdo {\n> +\t\terr = run_slot(slot);\n> +\t} while (err == HTTP_REAUTH && !large_request && !use_gzip);\n> +\tif (err != HTTP_OK)\n> +\t\terr = -1;\n>  \n>  \tcurl_slist_free_all(headers);\n>  \tfree(gzip_body);\n> diff --git a/t/t5541-http-push.sh b/t/t5541-http-push.sh\n> index 9b1cd60..ef6d6b6 100755\n> --- a/t/t5541-http-push.sh\n> +++ b/t/t5541-http-push.sh\n> @@ -280,7 +280,7 @@ test_expect_success 'push over smart http with auth' '\n>  \ttest_cmp expect actual\n>  '\n>  \n> -test_expect_failure 'push to auth-only-for-push repo' '\n> +test_expect_success 'push to auth-only-for-push repo' '\n>  \tcd \"$ROOT_PATH/test_repo_clone\" &&\n>  \techo push-half-auth >expect &&\n>  \ttest_commit push-half-auth &&\n"},{"id":"197937","messageId":"20120827214930.GA18287@sigill.intra.peff.net","threadId":"31349","inReplyTo":"7v3938rztf.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 8/8] http: prompt for credentials on failed POST","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-08-27T21:49:30Z","receivedAt":"2012-08-27T21:49:30Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Aug 27, 2012 at 10:48:28AM -0700, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > Most of the time, this is not a big deal; for both fetching\n> > and pushing, we make a GET request before doing any POSTs,\n> > so typically we figure out the credentials during the first\n> > request, then reuse them during the POST. However, some\n> > servers may allow a client to get the list of refs from\n> > receive-pack without authentication, and then require\n> > authentication when the client actually tries to POST the\n> > pack.\n> \n> A silly question.  Does the initial GET request when we push look\n> any different from the initial GET request when we fetch?  Can we\n> make them look different in an updated client, so that the server\n> side can say \"this GET is about pushing into us, and we require\n> authentication\"?\n\nYes, they are already different. A fetch asks for\n\n  info/refs?service=git-upload-pack\n\nand a push asks for\n\n  info/refs?service-git-receive-pack\n\nAnd I definitely think the optimal server config will authenticate the\nclient at that first GET step, because the client may do a significant\namount of work for the POST (due to the probe_rpc, it won't actually\n_send_ a large pack, but it will do the complete delta-compression phase\nbefore generating any output, which can be slow).\n\nBut doing it this way has been advertised in our manpage for so long, I\nassume some people are using it. And given that it used to work for\nolder clients (prior to v1.7.8), and that the person who upgraded their\nclient is not always in charge of telling the person running the server\nto fix their server, I think it's worth un-breaking it.\n\nAnd we should definitely tweak what git-http-backend advertises on top\nso that eventually this sub-optimal config dies out.\n\n> >   1. If we are using gzip. However, we only do so when\n> >      calling git-upload-pack, so it does not apply to\n> >      pushes.\n> >\n> >   2. If we have a large request, the probe succeeds, but\n> >      then the real POST wants authentication. This is an\n> >      extremely unlikely configuration and not worth worrying\n> >      about.\n> >\n> > While it might be nice to cover those instances, doing so\n> > would be significantly more complex for very little\n> > real-world gain. In the long run, we will be much better off\n> > when curl learns to internally handle authentication as a\n> > callback, and we can cleanly handle all cases that way.\n> \n> I suspect that in real life, almost nobody runs smart HTTP server\n> that allows anonymous push.\n> \n> How much usability penalty would it be if we always fill credential\n> before pushing?\n\nIt would reintroduce the problem that 986bbc0 was fixing: we would\nprompt even when curl would end up pulling the credential from .netrc.\nI find that somewhat less compelling a problem now that we have\ncredential helpers, though. And of course it does not fix (1) or (2)\nabove, either.\n\n> Alternatively, how much latency penalty would it incur if we always\n> send a probe request regardless of the request size when we try to\n> push without having an authentication material?\n\nIt would be one http round-trip and no-op invocation of request-pack on\nthe server. If we did it only on push, that would probably not be too\nbad, as we would hit it only when we were actually pushing something.\n\nBut that would still suffer from (1) and (2) above, so I don't see it as\na real advantage. You _could_ fix both cases by buffering the input data\nand restarting the request. I just didn't think it was worth doing,\nsince they are unlikely configurations and the code complexity is much\nhigher.\n\n-Peff\n"},{"id":"197955","messageId":"7voblvq5gw.fsf@alter.siamese.dyndns.org","threadId":"31349","inReplyTo":"20120827214930.GA18287@sigill.intra.peff.net","subject":"Re: [PATCH 8/8] http: prompt for credentials on failed POST","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-08-27T23:29:19Z","receivedAt":"2012-08-27T23:29:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n>> A silly question.  Does the initial GET request when we push look\n>> any different from the initial GET request when we fetch?  Can we\n>> make them look different in an updated client, so that the server\n>> side can say \"this GET is about pushing into us, and we require\n>> authentication\"?\n>\n> Yes, they are already different. A fetch asks for\n> ...\n> But doing it this way has been advertised in our manpage for so long, I\n> assume some people are using it. And given that it used to work for\n> older clients (prior to v1.7.8), and that the person who upgraded their\n> client is not always in charge of telling the person running the server\n> to fix their server, I think it's worth un-breaking it.\n\nOh, I wasn't saying the fix is unnecessary.  I was trying to see if\nthere is something people who _care_ about wasted effort on the\nclient side can do to fix their configuration properly (otherwise\nwhile we are patching the client, make sure we give them a way).\n\n> But that would still suffer from (1) and (2) above, so I don't see it as\n> a real advantage. You _could_ fix both cases by buffering the input data\n> and restarting the request. I just didn't think it was worth doing,\n> since they are unlikely configurations and the code complexity is much\n> higher.\n\nOK.\n"},{"id":"198006","messageId":"7vd32a28n7.fsf@alter.siamese.dyndns.org","threadId":"31349","inReplyTo":"20120827132604.GG17375@sigill.intra.peff.net","subject":"Re: [PATCH 7/8] http: factor out http error code handling","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-08-28T18:06:52Z","receivedAt":"2012-08-28T18:06:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> Most of our http requests go through the http_request()\n> interface, which does some nice post-processing on the\n> results. In particular, it handles prompting for missing\n> credentials as well as approving and rejecting valid or\n> invalid credentials. Unfortunately, it only handles GET\n> requests. Making it handle POSTs would be quite complex, so\n> let's pull result handling code into its own function so\n> that it can be reused from the POST code paths.\n>\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n>  http.c | 51 ++++++++++++++++++++++++++++-----------------------\n>  http.h |  1 +\n>  2 files changed, 29 insertions(+), 23 deletions(-)\n>\n> diff --git a/http.c b/http.c\n> index b61ac85..6793137 100644\n> --- a/http.c\n> +++ b/http.c\n> @@ -745,6 +745,33 @@ char *get_remote_object_url(const char *url, const char *hex,\n>  \treturn strbuf_detach(&buf, NULL);\n>  }\n>  \n> +int handle_curl_result(struct active_request_slot *slot)\n> +{\n> +\tstruct slot_results *results = slot->results;\n> +\n> +\tif (results->curl_result == CURLE_OK) {\n> +\t\tcredential_approve(&http_auth);\n> +\t\treturn HTTP_OK;\n> +\t} else if (missing_target(results))\n> +...\n> +\t\treturn HTTP_ERROR;\n> +\t}\n> +}\n> +\n> @@ -820,9 +828,6 @@ static int http_request(const char *url, void *result, int target, int options)\n>  \tcurl_slist_free_all(headers);\n>  \tstrbuf_release(&buf);\n>  \n> -\tif (ret == HTTP_OK)\n> -\t\tcredential_approve(&http_auth);\n\nOK, now this is part of handle_curl_result() so the caller does not\nhave to worry about it, which is nice ;-)\n\n>  \treturn ret;\n>  }\n"}]}