{"thread":{"id":"30121","subject":"Cannot clone the git repository shared over http with authorization.","startedAt":"2012-04-01T18:48:04Z","lastAt":"2012-04-13T06:19:25Z","messageCount":12,"participants":["Artur R. Czechowski","Clemens Buchacher","Jeff King","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"188280","messageId":"20120401184804.GJ3236@szczaw.snafu.pl","threadId":"30121","inReplyTo":null,"subject":"Cannot clone the git repository shared over http with authorization.","fromName":"Artur R. Czechowski","fromEmail":"arturcz@hell.pl","sentAt":"2012-04-01T18:48:04Z","receivedAt":"2012-04-01T18:48:04Z","isPatch":false,"sender":{"key":"arturcz@hell.pl","avatar":null},"body":"Hello,\n\narturcz@szczaw:/tmp$ git clone http://blabluga.hell.pl/git/test.git\nCloning into 'test'...\nDebug: Remote helper: -> capabilities\nDebug: Remote helper: Waiting...\nDebug: Remote helper: <- fetch\nDebug: Got cap fetch\nDebug: Remote helper: Waiting...\nDebug: Remote helper: <- option\nDebug: Got cap option\nDebug: Remote helper: Waiting...\nDebug: Remote helper: <- push\nDebug: Got cap push\nDebug: Remote helper: Waiting...\nDebug: Remote helper: <- \nDebug: Capabilities complete.\nDebug: Remote helper: Waiting...\nUsername for 'http://blabluga.hell.pl': test\nPassword for 'http://test@blabluga.hell.pl': \nDebug: Remote helper: <- @refs/heads/master HEAD\nDebug: Remote helper: Waiting...\nDebug: Remote helper: <- c64bcf957545f61436d405326d985521dc45058f refs/heads/master\nDebug: Remote helper: Waiting...\nDebug: Remote helper: <- \nDebug: Read ref listing.\nDebug: Remote helper: -> option progress true\nDebug: Remote helper: Waiting...\nDebug: Remote helper: <- ok\nDebug: Remote helper: -> option verbosity 1\nDebug: Remote helper: Waiting...\nDebug: Remote helper: <- ok\nDebug: Remote helper: -> fetch c64bcf957545f61436d405326d985521dc45058f HEAD\nfetch c64bcf957545f61436d405326d985521dc45058f refs/heads/master\n\nDebug: Remote helper: Waiting...\nerror: The requested URL returned error: 401 (curl_result = 22, http_code = 401, sha1 = e884293079beab9f2583b59b4e05479fc84fc588)\nerror: Unable to find e884293079beab9f2583b59b4e05479fc84fc588 under http://blabluga.hell.pl/git/test.git\nCannot obtain needed commit e884293079beab9f2583b59b4e05479fc84fc588\nwhile processing commit c64bcf957545f61436d405326d985521dc45058f.\nerror: Fetch failed.\n\nThe apache log from the action:\n\n192.168.109.13 - - [01/Apr/2012:20:33:42 +0200] \"GET /git/test.git/info/refs?service=git-upload-pack HTTP/1.1\" 401 694 \"-\" \"git/1.7.9.1\"\n192.168.109.13 - - [01/Apr/2012:20:33:50 +0200] \"GET /git/test.git/info/refs?service=git-upload-pack HTTP/1.1\" 401 694 \"-\" \"git/1.7.9.1\"\n192.168.109.13 - test [01/Apr/2012:20:33:50 +0200] \"GET /git/test.git/info/refs?service=git-upload-pack HTTP/1.1\" 200 294 \"-\" \"git/1.7.9.1\"\n192.168.109.13 - test [01/Apr/2012:20:33:50 +0200] \"GET /git/test.git/HEAD HTTP/1.1\" 200 257 \"-\" \"git/1.7.9.1\"\n192.168.109.13 - test [01/Apr/2012:20:33:50 +0200] \"GET /git/test.git/objects/c6/4bcf957545f61436d405326d985521dc45058f HTTP/1.1\" 200 403 \"-\" \"git/1.7.9.1\"\n192.168.109.13 - test [01/Apr/2012:20:33:50 +0200] \"GET /git/test.git/objects/b8/ab890a5432053f85d74a3b4ff798011260e222 HTTP/1.1\" 200 288 \"-\" \"git/1.7.9.1\"\n192.168.109.13 - - [01/Apr/2012:20:33:50 +0200] \"GET /git/test.git/objects/fe/0e1075a026f19269bd958b05f586e252af7635 HTTP/1.1\" 401 694 \"-\" \"git/1.7.9.1\"\n192.168.109.13 - - [01/Apr/2012:20:33:50 +0200] \"GET /git/test.git/objects/e8/84293079beab9f2583b59b4e05479fc84fc588 HTTP/1.1\" 401 694 \"-\" \"git/1.7.9.1\"\n192.168.109.13 - - [01/Apr/2012:20:33:50 +0200] \"GET /git/test.git/objects/info/packs HTTP/1.1\" 401 694 \"-\" \"git/1.7.9.1\"\n192.168.109.13 - - [01/Apr/2012:20:33:50 +0200] \"GET /git/test.git/objects/info/http-alternates HTTP/1.1\" 401 694 \"-\" \"git/1.7.9.1\"\n192.168.109.13 - - [01/Apr/2012:20:33:50 +0200] \"GET /git/test.git/objects/info/alternates HTTP/1.1\" 401 694 \"-\" \"git/1.7.9.1\"\n\nOn the client side I'm using 1.7.9.1 from Debian package. Repository is\nshared from apache using a DAV on directive (a person from #git channel\ntold me it's called a dumb http).\n\nIt's worth to say, that I've already asked for support on #git channel\nand the problem exists with git/1.7.9.5 and git/1.7.10.rc3.3.g19a6c (at\nleast those versions can be seen in my access.log).\n\nIf necessary I can arrange an access to the repository.\n\nRegards\n Artur\n-- \n(ac) ,< \n _รณ_____ We all live in a yellow submarine... \n <_______> \n"},{"id":"188281","messageId":"20120401194534.GA16512@ecki","threadId":"30121","inReplyTo":"20120401184804.GJ3236@szczaw.snafu.pl","subject":"Re: Cannot clone the git repository shared over http with authorization.","fromName":"Clemens Buchacher","fromEmail":"drizzd@aon.at","sentAt":"2012-04-01T19:45:36Z","receivedAt":"2012-04-01T19:45:36Z","isPatch":false,"sender":{"key":"drizzd@gmx.net","avatar":"https://avatars.githubusercontent.com/u/59082?v=4"},"body":"On Sun, Apr 01, 2012 at 08:48:04PM +0200, Artur R. Czechowski wrote:\n> \n> arturcz@szczaw:/tmp$ git clone http://blabluga.hell.pl/git/test.git\n> Cloning into 'test'...\n> error: The requested URL returned error: 401 (curl_result = 22, http_code = 401, sha1 = e884293079beab9f2583b59b4e05479fc84fc588)\n> error: Unable to find e884293079beab9f2583b59b4e05479fc84fc588 under http://blabluga.hell.pl/git/test.git\n> Cannot obtain needed commit e884293079beab9f2583b59b4e05479fc84fc588\n> while processing commit c64bcf957545f61436d405326d985521dc45058f.\n> error: Fetch failed.\n\nI've been looking at this a bit. It's probably worth mentioning that the\nproblem does _not_ happen if username and password are specified in the\nURL or in the .netrc. In both of those cases, curl is taking care of the\ncredentials itself.\n\nSo far I figured out that setting 'git config http.maxRequests 1' fixes\nthe problem as well. Looking at the output with GIT_CURL_VERBOSE=1 set,\nit seems that some GET requests use the credentials, while others do\nnot. My guess is that the CURLOPT_USERPWD option does not apply to all\nthreads.\n\nClemens\n"},{"id":"188283","messageId":"20120401205324.GA31046@ecki","threadId":"30121","inReplyTo":"20120401194534.GA16512@ecki","subject":"Re: Cannot clone the git repository shared over http with authorization.","fromName":"Clemens Buchacher","fromEmail":"drizzd@aon.at","sentAt":"2012-04-01T20:53:26Z","receivedAt":"2012-04-01T20:53:26Z","isPatch":false,"sender":{"key":"drizzd@gmx.net","avatar":"https://avatars.githubusercontent.com/u/59082?v=4"},"body":"On Sun, Apr 01, 2012 at 09:45:36PM +0200, Clemens Buchacher wrote:\n> On Sun, Apr 01, 2012 at 08:48:04PM +0200, Artur R. Czechowski wrote:\n> > \n> > arturcz@szczaw:/tmp$ git clone http://blabluga.hell.pl/git/test.git\n> > Cloning into 'test'...\n> > error: The requested URL returned error: 401 (curl_result = 22, http_code = 401, sha1 = e884293079beab9f2583b59b4e05479fc84fc588)\n> > error: Unable to find e884293079beab9f2583b59b4e05479fc84fc588 under http://blabluga.hell.pl/git/test.git\n> > Cannot obtain needed commit e884293079beab9f2583b59b4e05479fc84fc588\n> > while processing commit c64bcf957545f61436d405326d985521dc45058f.\n> > error: Fetch failed.\n\nThis is the minimal test case I could find to reproduce this reliably on\nin our test suite. It seems that a certain number of loose objects is\nalso required to trigger the issue.\n\ndiff --git a/t/lib-httpd/apache.conf b/t/lib-httpd/apache.conf\nindex 3c12b05..d31c083 100644\n--- a/t/lib-httpd/apache.conf\n+++ b/t/lib-httpd/apache.conf\n@@ -77,6 +77,13 @@ SSLMutex file:ssl_mutex\n SSLEngine On\n \n \n+\n+\tAuthType Basic\n+\tAuthName \"git-auth\"\n+\tAuthUserFile passwd\n+\tRequire valid-user\n+\n+\n \n \tAuthType Basic\n \tAuthName \"git-auth\"\ndiff --git a/t/t5550-http-fetch.sh b/t/t5550-http-fetch.sh\nindex e5e6b8f..3eb8753 100755\n--- a/t/t5550-http-fetch.sh\n+++ b/t/t5550-http-fetch.sh\n@@ -13,17 +13,22 @@ LIB_HTTPD_PORT=${LIB_HTTPD_PORT-'5550'}\n start_httpd\n \n test_expect_success 'setup repository' '\n-\techo content >file &&\n+\techo content1 >file &&\n \tgit add file &&\n \tgit commit -m one\n+\techo content2 >file &&\n+\tgit add file &&\n+\tgit commit -m two\n '\n \n test_expect_success 'create http-accessible bare repository' '\n-\tmkdir \"$HTTPD_DOCUMENT_ROOT_PATH/repo.git\" &&\n+\tcp -r .git \"$HTTPD_DOCUMENT_ROOT_PATH/repo.git\" &&\n \t(cd \"$HTTPD_DOCUMENT_ROOT_PATH/repo.git\" &&\n-\t git --bare init &&\n+\t git config core.bare true &&\n+\t mv hooks-disabled hooks &&\n \t echo \"exec git update-server-info\" >hooks/post-update &&\n-\t chmod +x hooks/post-update\n+\t chmod +x hooks/post-update &&\n+\t hooks/post-update\n \t) &&\n \tgit remote add public \"$HTTPD_DOCUMENT_ROOT_PATH/repo.git\" &&\n \tgit push public master:master\n@@ -73,6 +78,13 @@ expect_askpass() {\n \ttest_cmp askpass-expect askpass-query\n }\n \n+test_expect_success 'clone password-protected repository with dumb http' '\n+\t>askpass-query &&\n+\techo user@host >askpass-response &&\n+\tGIT_CURL_VERBOSE=1 git clone \"$HTTPD_URL/dumb/auth/repo.git\" repo-dumb-auth &&\n+\texpect_askpass both user@host\n+'\n+\n test_expect_success 'cloning password-protected repository can fail' '\n \t>askpass-query &&\n \techo wrong >askpass-response &&\n"},{"id":"188300","messageId":"20120402083115.GA2235@sigill.intra.peff.net","threadId":"30121","inReplyTo":"20120401194534.GA16512@ecki","subject":"Re: Cannot clone the git repository shared over http with authorization.","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-04-02T08:31:15Z","receivedAt":"2012-04-02T08:31:15Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Apr 01, 2012 at 09:45:36PM +0200, Clemens Buchacher wrote:\n\n> So far I figured out that setting 'git config http.maxRequests 1' fixes\n> the problem as well. Looking at the output with GIT_CURL_VERBOSE=1 set,\n> it seems that some GET requests use the credentials, while others do\n> not. My guess is that the CURLOPT_USERPWD option does not apply to all\n> threads.\n\nYes, that is exactly it. Furthermore, the multi-fetch code paths used by\nthe http walker do not have the magic \"notice a 401 and retry\". I think\nsomething like this should fix it (it passes your tests):\n\ndiff --git a/http.c b/http.c\nindex f3f83d7..c6dc9b7 100644\n--- a/http.c\n+++ b/http.c\n@@ -494,6 +494,8 @@ struct active_request_slot *get_active_slot(void)\n \tcurl_easy_setopt(slot->curl, CURLOPT_POSTFIELDS, NULL);\n \tcurl_easy_setopt(slot->curl, CURLOPT_UPLOAD, 0);\n \tcurl_easy_setopt(slot->curl, CURLOPT_HTTPGET, 1);\n+\tif (http_auth.password)\n+\t\tinit_curl_http_auth(slot->curl);\n \n \treturn slot;\n }\n\nwhich is basically just double-checking that we set CURLOPT_USERPWD\nwhenever we get a slot (I wish there was some way of asking \"does this\ncurl handle have USERPWD already set?\", but I couldn't fine one).\n\nIt still doesn't do 401-handling itself, but it works OK in practice\nbecause the first request will be a non-multi fetch (to get info/refs),\nwhich recognizes the 401 and prompts the user, and then that credential\ngets used subsequently.\n\n-Peff\n"},{"id":"188857","messageId":"1334051620-18044-1-git-send-email-drizzd@aon.at","threadId":"30121","inReplyTo":"20120402083115.GA2235@sigill.intra.peff.net","subject":"Re: Cannot clone the git repository shared over http with authorization.","fromName":"Clemens Buchacher","fromEmail":"drizzd@aon.at","sentAt":"2012-04-10T09:53:38Z","receivedAt":"2012-04-10T09:53:38Z","isPatch":false,"sender":{"key":"drizzd@gmx.net","avatar":"https://avatars.githubusercontent.com/u/59082?v=4"},"body":"On Mon, Apr 02, 2012 at 04:31:15AM -0400, Jeff King wrote:\n> On Sun, Apr 01, 2012 at 09:45:36PM +0200, Clemens Buchacher wrote:\n> \n> > So far I figured out that setting 'git config http.maxRequests 1' fixes\n> > the problem as well. Looking at the output with GIT_CURL_VERBOSE=1 set,\n> > it seems that some GET requests use the credentials, while others do\n> > not. My guess is that the CURLOPT_USERPWD option does not apply to all\n> > threads.\n> \n> Yes, that is exactly it. Furthermore, the multi-fetch code paths used by\n> the http walker do not have the magic \"notice a 401 and retry\". I think\n> something like this should fix it (it passes your tests),\n> which is basically just double-checking that we set CURLOPT_USERPWD\n> whenever we get a slot (I wish there was some way of asking \"does this\n> curl handle have USERPWD already set?\", but I couldn't fine one).\n\nI think this is the way to go. I am sending the updated patch for the test\n(much of my changes were unnecessary), and your fix on top.\n\n[PATCH 1/2] http auth fails with multiple curl handles\n[PATCH 2/2] fix http auth with multiple curl handles\n\nClemens\n"},{"id":"188856","messageId":"1334051620-18044-2-git-send-email-drizzd@aon.at","threadId":"30121","inReplyTo":"1334051620-18044-1-git-send-email-drizzd@aon.at","subject":"[PATCH 1/2] http auth fails with multiple curl handles","fromName":"Clemens Buchacher","fromEmail":"drizzd@aon.at","sentAt":"2012-04-10T09:53:39Z","receivedAt":"2012-04-10T09:53:39Z","isPatch":true,"sender":{"key":"drizzd@gmx.net","avatar":"https://avatars.githubusercontent.com/u/59082?v=4"},"body":"Create a repo with multiple loose objects in order to demonstrate http\nauthentication breakage.\n\nSigned-off-by: Clemens Buchacher \n---\n t/t5550-http-fetch.sh | 25 +++++++++++++++----------\n 1 file changed, 15 insertions(+), 10 deletions(-)\n\ndiff --git a/t/t5550-http-fetch.sh b/t/t5550-http-fetch.sh\nindex e5e6b8f..1d9ff1e 100755\n--- a/t/t5550-http-fetch.sh\n+++ b/t/t5550-http-fetch.sh\n@@ -13,17 +13,22 @@ LIB_HTTPD_PORT=${LIB_HTTPD_PORT-'5550'}\n start_httpd\n \n test_expect_success 'setup repository' '\n-\techo content >file &&\n+\techo content1 >file &&\n \tgit add file &&\n \tgit commit -m one\n+\techo content2 >file &&\n+\tgit add file &&\n+\tgit commit -m two\n '\n \n-test_expect_success 'create http-accessible bare repository' '\n-\tmkdir \"$HTTPD_DOCUMENT_ROOT_PATH/repo.git\" &&\n+test_expect_success 'create http-accessible bare repository with loose objects' '\n+\tcp -a .git \"$HTTPD_DOCUMENT_ROOT_PATH/repo.git\" &&\n \t(cd \"$HTTPD_DOCUMENT_ROOT_PATH/repo.git\" &&\n-\t git --bare init &&\n+\t git config core.bare true &&\n+\t mkdir -p hooks &&\n \t echo \"exec git update-server-info\" >hooks/post-update &&\n-\t chmod +x hooks/post-update\n+\t chmod +x hooks/post-update &&\n+\t hooks/post-update\n \t) &&\n \tgit remote add public \"$HTTPD_DOCUMENT_ROOT_PATH/repo.git\" &&\n \tgit push public master:master\n@@ -87,21 +92,21 @@ test_expect_success 'http auth can use user/pass in URL' '\n \texpect_askpass none\n '\n \n-test_expect_success 'http auth can use just user in URL' '\n+test_expect_failure '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 \texpect_askpass pass user@host\n '\n \n-test_expect_success 'http auth can request both user and pass' '\n+test_expect_failure '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 \texpect_askpass both user@host\n '\n \n-test_expect_success 'http auth respects credential helper config' '\n+test_expect_failure 'http auth respects credential helper config' '\n \ttest_config_global credential.helper \"!f() {\n \t\tcat >/dev/null\n \t\techo username=user@host\n@@ -113,7 +118,7 @@ test_expect_success 'http auth respects credential helper config' '\n \texpect_askpass none\n '\n \n-test_expect_success 'http auth can get username from config' '\n+test_expect_failure '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@@ -121,7 +126,7 @@ test_expect_success 'http auth can get username from config' '\n \texpect_askpass pass user@host\n '\n \n-test_expect_success 'configured username does not override URL' '\n+test_expect_failure '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-- \n1.7.9.6\n"},{"id":"188858","messageId":"1334051620-18044-3-git-send-email-drizzd@aon.at","threadId":"30121","inReplyTo":"1334051620-18044-1-git-send-email-drizzd@aon.at","subject":"[PATCH 2/2] fix http auth with multiple curl handles","fromName":"Clemens Buchacher","fromEmail":"drizzd@aon.at","sentAt":"2012-04-10T09:53:40Z","receivedAt":"2012-04-10T09:53:40Z","isPatch":true,"sender":{"key":"drizzd@gmx.net","avatar":"https://avatars.githubusercontent.com/u/59082?v=4"},"body":"From: Jeff King \n\nHTTP authentication is currently handled by get_refs and fetch_ref, but\nnot by fetch_object, fetch_pack or fetch_alternates. In the\nsingle-threaded case, this is not an issue, since get_refs is always\ncalled first. It recognigzes the 401 and prompts the user for\ncredentials, which will then be used subsequently.\n\nIf the curl multi interface is used, however, only the multi handle used\nby get_refs will have credentials configured. Requests made by other\nhandles fail with an authentication error.\n\nFix this by setting CURLOPT_USERPWD whenever a slot is requested.\n\nSigned-off-by: Clemens Buchacher \n---\n http.c | 2 ++\n t/t5550-http-fetch.sh | 10 +++++-----\n 2 files changed, 7 insertions(+), 5 deletions(-)\n\ndiff --git a/http.c b/http.c\nindex f3f83d7..c6dc9b7 100644\n--- a/http.c\n+++ b/http.c\n@@ -494,6 +494,8 @@ struct active_request_slot *get_active_slot(void)\n \tcurl_easy_setopt(slot->curl, CURLOPT_POSTFIELDS, NULL);\n \tcurl_easy_setopt(slot->curl, CURLOPT_UPLOAD, 0);\n \tcurl_easy_setopt(slot->curl, CURLOPT_HTTPGET, 1);\n+\tif (http_auth.password)\n+\t\tinit_curl_http_auth(slot->curl);\n \n \treturn slot;\n }\ndiff --git a/t/t5550-http-fetch.sh b/t/t5550-http-fetch.sh\nindex 1d9ff1e..b06f817 100755\n--- a/t/t5550-http-fetch.sh\n+++ b/t/t5550-http-fetch.sh\n@@ -92,21 +92,21 @@ test_expect_success 'http auth can use user/pass in URL' '\n \texpect_askpass none\n '\n \n-test_expect_failure 'http auth can use just user in URL' '\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 \texpect_askpass pass user@host\n '\n \n-test_expect_failure 'http auth can request both user and pass' '\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 \texpect_askpass both user@host\n '\n \n-test_expect_failure 'http auth respects credential helper config' '\n+test_expect_success 'http auth respects credential helper config' '\n \ttest_config_global credential.helper \"!f() {\n \t\tcat >/dev/null\n \t\techo username=user@host\n@@ -118,7 +118,7 @@ test_expect_failure 'http auth respects credential helper config' '\n \texpect_askpass none\n '\n \n-test_expect_failure 'http auth can get username from config' '\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@@ -126,7 +126,7 @@ test_expect_failure 'http auth can get username from config' '\n \texpect_askpass pass user@host\n '\n \n-test_expect_failure 'configured username does not override URL' '\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-- \n1.7.9.6\n"},{"id":"189082","messageId":"20120412070910.GA31122@sigill.intra.peff.net","threadId":"30121","inReplyTo":"1334051620-18044-3-git-send-email-drizzd@aon.at","subject":"Re: [PATCH 2/2] fix http auth with multiple curl handles","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-04-12T07:09:10Z","receivedAt":"2012-04-12T07:09:10Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Apr 10, 2012 at 11:53:40AM +0200, Clemens Buchacher wrote:\n\n> HTTP authentication is currently handled by get_refs and fetch_ref, but\n> not by fetch_object, fetch_pack or fetch_alternates. In the\n> single-threaded case, this is not an issue, since get_refs is always\n> called first. It recognigzes the 401 and prompts the user for\n> credentials, which will then be used subsequently.\n> \n> If the curl multi interface is used, however, only the multi handle used\n> by get_refs will have credentials configured. Requests made by other\n> handles fail with an authentication error.\n> \n> Fix this by setting CURLOPT_USERPWD whenever a slot is requested.\n\nThe reason I didn't like my initial patch was that by calling\ncurl_easy_setopt() for every request, we end up leaking the password\nbuffer once per request.\n\nUnfortunately, I don't think there's a way to ask curl whether it has\nanything in CURLOPT_USERPWD. So we have to overwrite. Recent versions of\ncurl will actually copy the string we give it anyway, so the existing\ncode is already leaking a little bit (but once per process, not once per\nrequest). I wish we could get away with just handing curl the data and\nassuming it would copy, but that code came about in curl 7.17.0, in\n2007. According to our #if statements, we handle much older versions of\ncurl, so that is a non-option.\n\nI think the best we can do is to put the auth data in a static buffer\nand feed that to curl. We end up rewriting the auth data into our buffer\nover and over, but at least we don't re-malloc it. Like this:\n\ndiff --git a/http.c b/http.c\nindex f3f83d7..374c3bb 100644\n--- a/http.c\n+++ b/http.c\n@@ -211,12 +211,12 @@ static int http_options(const char *var, const char *value, void *cb)\n static void init_curl_http_auth(CURL *result)\n {\n \tif (http_auth.username) {\n-\t\tstruct strbuf up = STRBUF_INIT;\n+\t\tstatic struct strbuf up = STRBUF_INIT;\n \t\tcredential_fill(&http_auth);\n+\t\tstrbuf_reset(&up);\n \t\tstrbuf_addf(&up, \"%s:%s\",\n \t\t\t http_auth.username, http_auth.password);\n-\t\tcurl_easy_setopt(result, CURLOPT_USERPWD,\n-\t\t\t\t strbuf_detach(&up, NULL));\n+\t\tcurl_easy_setopt(result, CURLOPT_USERPWD, up.buf);\n \t}\n }\n \n\nBy the way, this touches on an area that I noticed while refactoring the\nhttp auth code a while ago, but decided not to tackle at the time. We\nfill in the auth information early, and then never bother to revisit it\nas URLs change. So for example, if I got a redirect from host A to host\nB, we would continue to use the credential for host A and host B. Which\nis maybe convenient, and maybe a security issue.\n\nIt has been that way since the beginning of git, and nobody has seemed\nto care. So maybe it is not worth dealing with. But if we did want to,\nthe right way would be to keep several credentials on hand, and match\neach URL to them as we were about to request it. That would also provide\na fix to the problem we are fixing here. I don't know if it is worth\ndoing or not.\n\n-Peff\n"},{"id":"189156","messageId":"7v4nso4lb3.fsf@alter.siamese.dyndns.org","threadId":"30121","inReplyTo":"20120412070910.GA31122@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] fix http auth with multiple curl handles","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-04-12T22:50:08Z","receivedAt":"2012-04-12T22:50:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> ...\n> I think the best we can do is to put the auth data in a static buffer\n> and feed that to curl. We end up rewriting the auth data into our buffer\n> over and over, but at least we don't re-malloc it. Like this:\n>\n> diff --git a/http.c b/http.c\n> index f3f83d7..374c3bb 100644\n> --- a/http.c\n> +++ b/http.c\n> @@ -211,12 +211,12 @@ static int http_options(const char *var, const char *value, void *cb)\n> static void init_curl_http_auth(CURL *result)\n> {\n> \tif (http_auth.username) {\n> -\t\tstruct strbuf up = STRBUF_INIT;\n> +\t\tstatic struct strbuf up = STRBUF_INIT;\n> \t\tcredential_fill(&http_auth);\n> +\t\tstrbuf_reset(&up);\n> \t\tstrbuf_addf(&up, \"%s:%s\",\n> \t\t\t http_auth.username, http_auth.password);\n> -\t\tcurl_easy_setopt(result, CURLOPT_USERPWD,\n> -\t\t\t\t strbuf_detach(&up, NULL));\n> +\t\tcurl_easy_setopt(result, CURLOPT_USERPWD, up.buf);\n> \t}\n> }\n\nYeah, that's sad but I agree that is probably the best we could do. Do\nyou want me to squash it in?\n\n> By the way, this touches on an area that I noticed while refactoring the\n> http auth code a while ago, but decided not to tackle at the time. We\n> fill in the auth information early, and then never bother to revisit it\n> as URLs change. So for example, if I got a redirect from host A to host\n> B, we would continue to use the credential for host A and host B. Which\n> is maybe convenient, and maybe a security issue.\n\nGood point. Do we follow redirects, though?\n"},{"id":"189173","messageId":"20120413061622.GA27591@sigill.intra.peff.net","threadId":"30121","inReplyTo":"7v4nso4lb3.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 2/2] fix http auth with multiple curl handles","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-04-13T06:16:22Z","receivedAt":"2012-04-13T06:16:22Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Apr 12, 2012 at 03:50:08PM -0700, Junio C Hamano wrote:\n\n> > diff --git a/http.c b/http.c\n> > index f3f83d7..374c3bb 100644\n> > --- a/http.c\n> > +++ b/http.c\n> > @@ -211,12 +211,12 @@ static int http_options(const char *var, const char *value, void *cb)\n> > static void init_curl_http_auth(CURL *result)\n> > {\n> > \tif (http_auth.username) {\n> > -\t\tstruct strbuf up = STRBUF_INIT;\n> > +\t\tstatic struct strbuf up = STRBUF_INIT;\n> > \t\tcredential_fill(&http_auth);\n> > +\t\tstrbuf_reset(&up);\n> > \t\tstrbuf_addf(&up, \"%s:%s\",\n> > \t\t\t http_auth.username, http_auth.password);\n> > -\t\tcurl_easy_setopt(result, CURLOPT_USERPWD,\n> > -\t\t\t\t strbuf_detach(&up, NULL));\n> > +\t\tcurl_easy_setopt(result, CURLOPT_USERPWD, up.buf);\n> > \t}\n> > }\n> \n> Yeah, that's sad but I agree that is probably the best we could do. Do\n> you want me to squash it in?\n\nWe can use an #if-macro and only do the static thing for older versions\nof curl (newer versions copy for us). But since we'd have to carry the\nold code anyway, it doesn't clean up much. However, since there is an\neven newer interface for giving curl the credential, it might be worth\nconditionally including that.\n\nI'd rather not squash this bit. It's sufficiently subtle that I'd rather\ndo it on top as a separate patch. Plus I gotta keep up my shortlog\nstats. ;)\n\nSo here are the two patches I think we should apply on top of what you\nhave in ct/http-multi-curl-auth.\n\n [1/2]: http: clean up leak in init_curl_http_auth\n [2/2]: http: use newer curl options for setting credentials\n\n> > By the way, this touches on an area that I noticed while refactoring the\n> > http auth code a while ago, but decided not to tackle at the time. We\n> > fill in the auth information early, and then never bother to revisit it\n> > as URLs change. So for example, if I got a redirect from host A to host\n> > B, we would continue to use the credential for host A and host B. Which\n> > is maybe convenient, and maybe a security issue.\n> \n> Good point. Do we follow redirects, though?\n\nCurl follows them for us, since we set CURLOPT_FOLLOWLOCATION. However,\nit does say this under CURLOPT_USERPWD (which I somehow missed the last\ntime I looked into this issue):\n\n When using HTTP and CURLOPT_FOLLOWLOCATION, libcurl might perform\n several requests to possibly different hosts. libcurl will only send\n this user and password information to hosts using the initial host\n name (unless CURLOPT_UNRESTRICTED_AUTH is set), so if libcurl\n follows locations to other hosts it will not send the user and\n password to those. This is enforced to prevent accidental information\n leakage.\n\nSo it looks like we are already doing the safe thing (unless we redirect\nourselves outside of curl, but I don't think we ever do so). We won't\nproperly retry auth if we get a 401 on a redirect (we never retry auth\nif we already had both a username and password; we complain and exit).\nBut nobody has been complaining about that, so I guess it is a\nnon-issue.\n\n-Peff\n"},{"id":"189174","messageId":"20120413061835.GA13690@sigill.intra.peff.net","threadId":"30121","inReplyTo":"20120413061622.GA27591@sigill.intra.peff.net","subject":"[PATCH 1/2] http: clean up leak in init_curl_http_auth","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-04-13T06:18:35Z","receivedAt":"2012-04-13T06:18:35Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When we have a credential to give to curl, we must copy it\ninto a \"user:pass\" buffer and then hand the buffer to curl.\nOld versions of curl did not copy the buffer, and we were\nexpected to keep it valid. Newer versions of curl will copy\nthe buffer.\n\nOur solution was to use a strbuf and detach it, giving\nownership of the resulting buffer to curl. However, this\nmeant that we were leaking the buffer on newer versions of\ncurl, since curl was just copying it and throwing away the\nstring we passed. Furthermore, when we replaced a\ncredential (e.g., because our original one was rejected), we\nwere also leaking on both old and new versions of curl.\n\nThis got even worse in the last patch, which started\nreplacing the credential (and thus leaking) on every http\nrequest.\n\nInstead, let's use a static buffer to make the ownership\nmore clear and less leaky. We already keep a static \"struct\ncredential\", so we are only handling a single credential at\na time, anyway.\n\nSigned-off-by: Jeff King \n---\nThe \"old\" version was prior to 7.17.0, from 2007. That is probably still\nnew enough to care about. But at some point we should probably make a\nsweep through http.c and get rid of conditional blocks for really old\ncruft.\n\n http.c | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/http.c b/http.c\nindex c6dc9b7..eaf7f40 100644\n--- a/http.c\n+++ b/http.c\n@@ -211,12 +211,12 @@ static int http_options(const char *var, const char *value, void *cb)\n static void init_curl_http_auth(CURL *result)\n {\n \tif (http_auth.username) {\n-\t\tstruct strbuf up = STRBUF_INIT;\n+\t\tstatic struct strbuf up = STRBUF_INIT;\n \t\tcredential_fill(&http_auth);\n+\t\tstrbuf_reset(&up);\n \t\tstrbuf_addf(&up, \"%s:%s\",\n \t\t\t http_auth.username, http_auth.password);\n-\t\tcurl_easy_setopt(result, CURLOPT_USERPWD,\n-\t\t\t\t strbuf_detach(&up, NULL));\n+\t\tcurl_easy_setopt(result, CURLOPT_USERPWD, up.buf);\n \t}\n }\n \n-- \n1.7.9.6.6.g6b3b56\n"},{"id":"189175","messageId":"20120413061925.GB13690@sigill.intra.peff.net","threadId":"30121","inReplyTo":"20120413061622.GA27591@sigill.intra.peff.net","subject":"[PATCH 2/2] http: use newer curl options for setting credentials","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-04-13T06:19:25Z","receivedAt":"2012-04-13T06:19:25Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We give the username and password to curl by sticking them\nin a buffer of the form \"user:pass\" and handing the result\nto CURLOPT_USERPWD. Since curl 7.19.1, there is a split\nmechanism, where you can specify each element individually.\n\nThis has the advantage that a username can contain a \":\"\ncharacter. It also is less code for us, since we can hand\nour strings over to curl directly. And since curl 7.17.0 and\nhigher promise to copy the strings for us, we we don't even\nhave to worry about memory ownership issues.\n\nUnfortunately, we have to keep the ugly code for old curl\naround, but as it is now nicely #if'd out, we can easily get\nrid of it when we decide that 7.19.1 is \"old enough\".\n\nSigned-off-by: Jeff King \n---\nFor reference, 7.19.1 is from late 2008.\n\n http.c | 13 +++++++++++--\n 1 file changed, 11 insertions(+), 2 deletions(-)\n\ndiff --git a/http.c b/http.c\nindex eaf7f40..2ec3789 100644\n--- a/http.c\n+++ b/http.c\n@@ -210,14 +210,23 @@ static int http_options(const char *var, const char *value, void *cb)\n \n static void init_curl_http_auth(CURL *result)\n {\n-\tif (http_auth.username) {\n+\tif (!http_auth.username)\n+\t\treturn;\n+\n+\tcredential_fill(&http_auth);\n+\n+#if LIBCURL_VERSION_NUM >= 0x071301\n+\tcurl_easy_setopt(result, CURLOPT_USERNAME, http_auth.username);\n+\tcurl_easy_setopt(result, CURLOPT_PASSWORD, http_auth.password);\n+#else\n+\t{\n \t\tstatic struct strbuf up = STRBUF_INIT;\n-\t\tcredential_fill(&http_auth);\n \t\tstrbuf_reset(&up);\n \t\tstrbuf_addf(&up, \"%s:%s\",\n \t\t\t http_auth.username, http_auth.password);\n \t\tcurl_easy_setopt(result, CURLOPT_USERPWD, up.buf);\n \t}\n+#endif\n }\n \n static int has_cert_password(void)\n-- \n1.7.9.6.6.g6b3b56\n"}]}