* Cannot clone the git repository shared over http with authorization. @ 2012-04-01 18:48 Artur R. Czechowski 2012-04-01 19:45 ` Clemens Buchacher 0 siblings, 1 reply; 12+ messages in thread From: Artur R. Czechowski @ 2012-04-01 18:48 UTC (permalink / raw) To: git [-- Attachment #1: Type: text/plain, Size: 3890 bytes --] Hello, arturcz@szczaw:/tmp$ git clone http://blabluga.hell.pl/git/test.git Cloning into 'test'... Debug: Remote helper: -> capabilities Debug: Remote helper: Waiting... Debug: Remote helper: <- fetch Debug: Got cap fetch Debug: Remote helper: Waiting... Debug: Remote helper: <- option Debug: Got cap option Debug: Remote helper: Waiting... Debug: Remote helper: <- push Debug: Got cap push Debug: Remote helper: Waiting... Debug: Remote helper: <- Debug: Capabilities complete. Debug: Remote helper: Waiting... Username for 'http://blabluga.hell.pl': test Password for 'http://test@blabluga.hell.pl': Debug: Remote helper: <- @refs/heads/master HEAD Debug: Remote helper: Waiting... Debug: Remote helper: <- c64bcf957545f61436d405326d985521dc45058f refs/heads/master Debug: Remote helper: Waiting... Debug: Remote helper: <- Debug: Read ref listing. Debug: Remote helper: -> option progress true Debug: Remote helper: Waiting... Debug: Remote helper: <- ok Debug: Remote helper: -> option verbosity 1 Debug: Remote helper: Waiting... Debug: Remote helper: <- ok Debug: Remote helper: -> fetch c64bcf957545f61436d405326d985521dc45058f HEAD fetch c64bcf957545f61436d405326d985521dc45058f refs/heads/master Debug: Remote helper: Waiting... error: The requested URL returned error: 401 (curl_result = 22, http_code = 401, sha1 = e884293079beab9f2583b59b4e05479fc84fc588) error: Unable to find e884293079beab9f2583b59b4e05479fc84fc588 under http://blabluga.hell.pl/git/test.git Cannot obtain needed commit e884293079beab9f2583b59b4e05479fc84fc588 while processing commit c64bcf957545f61436d405326d985521dc45058f. error: Fetch failed. The apache log from the action: 192.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" 192.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" 192.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" 192.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" 192.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" 192.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" 192.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" 192.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" 192.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" 192.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" 192.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" On the client side I'm using 1.7.9.1 from Debian package. Repository is shared from apache using a DAV on directive (a person from #git channel told me it's called a dumb http). It's worth to say, that I've already asked for support on #git channel and the problem exists with git/1.7.9.5 and git/1.7.10.rc3.3.g19a6c (at least those versions can be seen in my access.log). If necessary I can arrange an access to the repository. Regards Artur -- (ac) ,< _ó_____ We all live in a yellow submarine... <_______> [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 198 bytes --] ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: Cannot clone the git repository shared over http with authorization. 2012-04-01 18:48 Cannot clone the git repository shared over http with authorization Artur R. Czechowski @ 2012-04-01 19:45 ` Clemens Buchacher 2012-04-01 20:53 ` Clemens Buchacher 2012-04-02 8:31 ` Jeff King 0 siblings, 2 replies; 12+ messages in thread From: Clemens Buchacher @ 2012-04-01 19:45 UTC (permalink / raw) To: Artur R. Czechowski; +Cc: git On Sun, Apr 01, 2012 at 08:48:04PM +0200, Artur R. Czechowski wrote: > > arturcz@szczaw:/tmp$ git clone http://blabluga.hell.pl/git/test.git > Cloning into 'test'... > error: The requested URL returned error: 401 (curl_result = 22, http_code = 401, sha1 = e884293079beab9f2583b59b4e05479fc84fc588) > error: Unable to find e884293079beab9f2583b59b4e05479fc84fc588 under http://blabluga.hell.pl/git/test.git > Cannot obtain needed commit e884293079beab9f2583b59b4e05479fc84fc588 > while processing commit c64bcf957545f61436d405326d985521dc45058f. > error: Fetch failed. I've been looking at this a bit. It's probably worth mentioning that the problem does _not_ happen if username and password are specified in the URL or in the .netrc. In both of those cases, curl is taking care of the credentials itself. So far I figured out that setting 'git config http.maxRequests 1' fixes the problem as well. Looking at the output with GIT_CURL_VERBOSE=1 set, it seems that some GET requests use the credentials, while others do not. My guess is that the CURLOPT_USERPWD option does not apply to all threads. Clemens ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: Cannot clone the git repository shared over http with authorization. 2012-04-01 19:45 ` Clemens Buchacher @ 2012-04-01 20:53 ` Clemens Buchacher 2012-04-02 8:31 ` Jeff King 1 sibling, 0 replies; 12+ messages in thread From: Clemens Buchacher @ 2012-04-01 20:53 UTC (permalink / raw) To: Artur R. Czechowski; +Cc: git On Sun, Apr 01, 2012 at 09:45:36PM +0200, Clemens Buchacher wrote: > On Sun, Apr 01, 2012 at 08:48:04PM +0200, Artur R. Czechowski wrote: > > > > arturcz@szczaw:/tmp$ git clone http://blabluga.hell.pl/git/test.git > > Cloning into 'test'... > > error: The requested URL returned error: 401 (curl_result = 22, http_code = 401, sha1 = e884293079beab9f2583b59b4e05479fc84fc588) > > error: Unable to find e884293079beab9f2583b59b4e05479fc84fc588 under http://blabluga.hell.pl/git/test.git > > Cannot obtain needed commit e884293079beab9f2583b59b4e05479fc84fc588 > > while processing commit c64bcf957545f61436d405326d985521dc45058f. > > error: Fetch failed. This is the minimal test case I could find to reproduce this reliably on in our test suite. It seems that a certain number of loose objects is also required to trigger the issue. diff --git a/t/lib-httpd/apache.conf b/t/lib-httpd/apache.conf index 3c12b05..d31c083 100644 --- a/t/lib-httpd/apache.conf +++ b/t/lib-httpd/apache.conf @@ -77,6 +77,13 @@ SSLMutex file:ssl_mutex SSLEngine On </IfDefine> +<Location /dumb/auth/> + AuthType Basic + AuthName "git-auth" + AuthUserFile passwd + Require valid-user +</Location> + <Location /auth/> AuthType Basic AuthName "git-auth" diff --git a/t/t5550-http-fetch.sh b/t/t5550-http-fetch.sh index e5e6b8f..3eb8753 100755 --- a/t/t5550-http-fetch.sh +++ b/t/t5550-http-fetch.sh @@ -13,17 +13,22 @@ LIB_HTTPD_PORT=${LIB_HTTPD_PORT-'5550'} start_httpd test_expect_success 'setup repository' ' - echo content >file && + echo content1 >file && git add file && git commit -m one + echo content2 >file && + git add file && + git commit -m two ' test_expect_success 'create http-accessible bare repository' ' - mkdir "$HTTPD_DOCUMENT_ROOT_PATH/repo.git" && + cp -r .git "$HTTPD_DOCUMENT_ROOT_PATH/repo.git" && (cd "$HTTPD_DOCUMENT_ROOT_PATH/repo.git" && - git --bare init && + git config core.bare true && + mv hooks-disabled hooks && echo "exec git update-server-info" >hooks/post-update && - chmod +x hooks/post-update + chmod +x hooks/post-update && + hooks/post-update ) && git remote add public "$HTTPD_DOCUMENT_ROOT_PATH/repo.git" && git push public master:master @@ -73,6 +78,13 @@ expect_askpass() { test_cmp askpass-expect askpass-query } +test_expect_success 'clone password-protected repository with dumb http' ' + >askpass-query && + echo user@host >askpass-response && + GIT_CURL_VERBOSE=1 git clone "$HTTPD_URL/dumb/auth/repo.git" repo-dumb-auth && + expect_askpass both user@host +' + test_expect_success 'cloning password-protected repository can fail' ' >askpass-query && echo wrong >askpass-response && ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: Cannot clone the git repository shared over http with authorization. 2012-04-01 19:45 ` Clemens Buchacher 2012-04-01 20:53 ` Clemens Buchacher @ 2012-04-02 8:31 ` Jeff King 2012-04-10 9:53 ` Clemens Buchacher 1 sibling, 1 reply; 12+ messages in thread From: Jeff King @ 2012-04-02 8:31 UTC (permalink / raw) To: Clemens Buchacher; +Cc: Artur R. Czechowski, git On Sun, Apr 01, 2012 at 09:45:36PM +0200, Clemens Buchacher wrote: > So far I figured out that setting 'git config http.maxRequests 1' fixes > the problem as well. Looking at the output with GIT_CURL_VERBOSE=1 set, > it seems that some GET requests use the credentials, while others do > not. My guess is that the CURLOPT_USERPWD option does not apply to all > threads. Yes, that is exactly it. Furthermore, the multi-fetch code paths used by the http walker do not have the magic "notice a 401 and retry". I think something like this should fix it (it passes your tests): diff --git a/http.c b/http.c index f3f83d7..c6dc9b7 100644 --- a/http.c +++ b/http.c @@ -494,6 +494,8 @@ struct active_request_slot *get_active_slot(void) curl_easy_setopt(slot->curl, CURLOPT_POSTFIELDS, NULL); curl_easy_setopt(slot->curl, CURLOPT_UPLOAD, 0); curl_easy_setopt(slot->curl, CURLOPT_HTTPGET, 1); + if (http_auth.password) + init_curl_http_auth(slot->curl); return slot; } which is basically just double-checking that we set CURLOPT_USERPWD whenever we get a slot (I wish there was some way of asking "does this curl handle have USERPWD already set?", but I couldn't fine one). It still doesn't do 401-handling itself, but it works OK in practice because the first request will be a non-multi fetch (to get info/refs), which recognizes the 401 and prompts the user, and then that credential gets used subsequently. -Peff ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: Cannot clone the git repository shared over http with authorization. 2012-04-02 8:31 ` Jeff King @ 2012-04-10 9:53 ` Clemens Buchacher 2012-04-10 9:53 ` [PATCH 1/2] http auth fails with multiple curl handles Clemens Buchacher 2012-04-10 9:53 ` [PATCH 2/2] fix http auth " Clemens Buchacher 0 siblings, 2 replies; 12+ messages in thread From: Clemens Buchacher @ 2012-04-10 9:53 UTC (permalink / raw) To: git; +Cc: Junio C Hamano, Jeff King On Mon, Apr 02, 2012 at 04:31:15AM -0400, Jeff King wrote: > On Sun, Apr 01, 2012 at 09:45:36PM +0200, Clemens Buchacher wrote: > > > So far I figured out that setting 'git config http.maxRequests 1' fixes > > the problem as well. Looking at the output with GIT_CURL_VERBOSE=1 set, > > it seems that some GET requests use the credentials, while others do > > not. My guess is that the CURLOPT_USERPWD option does not apply to all > > threads. > > Yes, that is exactly it. Furthermore, the multi-fetch code paths used by > the http walker do not have the magic "notice a 401 and retry". I think > something like this should fix it (it passes your tests), > which is basically just double-checking that we set CURLOPT_USERPWD > whenever we get a slot (I wish there was some way of asking "does this > curl handle have USERPWD already set?", but I couldn't fine one). I think this is the way to go. I am sending the updated patch for the test (much of my changes were unnecessary), and your fix on top. [PATCH 1/2] http auth fails with multiple curl handles [PATCH 2/2] fix http auth with multiple curl handles Clemens ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 1/2] http auth fails with multiple curl handles 2012-04-10 9:53 ` Clemens Buchacher @ 2012-04-10 9:53 ` Clemens Buchacher 2012-04-10 9:53 ` [PATCH 2/2] fix http auth " Clemens Buchacher 1 sibling, 0 replies; 12+ messages in thread From: Clemens Buchacher @ 2012-04-10 9:53 UTC (permalink / raw) To: git; +Cc: Junio C Hamano, Jeff King Create a repo with multiple loose objects in order to demonstrate http authentication breakage. Signed-off-by: Clemens Buchacher <drizzd@aon.at> --- t/t5550-http-fetch.sh | 25 +++++++++++++++---------- 1 file changed, 15 insertions(+), 10 deletions(-) diff --git a/t/t5550-http-fetch.sh b/t/t5550-http-fetch.sh index e5e6b8f..1d9ff1e 100755 --- a/t/t5550-http-fetch.sh +++ b/t/t5550-http-fetch.sh @@ -13,17 +13,22 @@ LIB_HTTPD_PORT=${LIB_HTTPD_PORT-'5550'} start_httpd test_expect_success 'setup repository' ' - echo content >file && + echo content1 >file && git add file && git commit -m one + echo content2 >file && + git add file && + git commit -m two ' -test_expect_success 'create http-accessible bare repository' ' - mkdir "$HTTPD_DOCUMENT_ROOT_PATH/repo.git" && +test_expect_success 'create http-accessible bare repository with loose objects' ' + cp -a .git "$HTTPD_DOCUMENT_ROOT_PATH/repo.git" && (cd "$HTTPD_DOCUMENT_ROOT_PATH/repo.git" && - git --bare init && + git config core.bare true && + mkdir -p hooks && echo "exec git update-server-info" >hooks/post-update && - chmod +x hooks/post-update + chmod +x hooks/post-update && + hooks/post-update ) && git remote add public "$HTTPD_DOCUMENT_ROOT_PATH/repo.git" && git push public master:master @@ -87,21 +92,21 @@ test_expect_success 'http auth can use user/pass in URL' ' expect_askpass none ' -test_expect_success 'http auth can use just user in URL' ' +test_expect_failure 'http auth can use just user in URL' ' >askpass-query && echo user@host >askpass-response && git clone "$HTTPD_URL_USER/auth/repo.git" clone-auth-pass && expect_askpass pass user@host ' -test_expect_success 'http auth can request both user and pass' ' +test_expect_failure 'http auth can request both user and pass' ' >askpass-query && echo user@host >askpass-response && git clone "$HTTPD_URL/auth/repo.git" clone-auth-both && expect_askpass both user@host ' -test_expect_success 'http auth respects credential helper config' ' +test_expect_failure 'http auth respects credential helper config' ' test_config_global credential.helper "!f() { cat >/dev/null echo username=user@host @@ -113,7 +118,7 @@ test_expect_success 'http auth respects credential helper config' ' expect_askpass none ' -test_expect_success 'http auth can get username from config' ' +test_expect_failure 'http auth can get username from config' ' test_config_global "credential.$HTTPD_URL.username" user@host && >askpass-query && echo user@host >askpass-response && @@ -121,7 +126,7 @@ test_expect_success 'http auth can get username from config' ' expect_askpass pass user@host ' -test_expect_success 'configured username does not override URL' ' +test_expect_failure 'configured username does not override URL' ' test_config_global "credential.$HTTPD_URL.username" wrong && >askpass-query && echo user@host >askpass-response && -- 1.7.9.6 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH 2/2] fix http auth with multiple curl handles 2012-04-10 9:53 ` Clemens Buchacher 2012-04-10 9:53 ` [PATCH 1/2] http auth fails with multiple curl handles Clemens Buchacher @ 2012-04-10 9:53 ` Clemens Buchacher 2012-04-12 7:09 ` Jeff King 1 sibling, 1 reply; 12+ messages in thread From: Clemens Buchacher @ 2012-04-10 9:53 UTC (permalink / raw) To: git; +Cc: Junio C Hamano, Jeff King From: Jeff King <peff@peff.net> HTTP authentication is currently handled by get_refs and fetch_ref, but not by fetch_object, fetch_pack or fetch_alternates. In the single-threaded case, this is not an issue, since get_refs is always called first. It recognigzes the 401 and prompts the user for credentials, which will then be used subsequently. If the curl multi interface is used, however, only the multi handle used by get_refs will have credentials configured. Requests made by other handles fail with an authentication error. Fix this by setting CURLOPT_USERPWD whenever a slot is requested. Signed-off-by: Clemens Buchacher <drizzd@aon.at> --- http.c | 2 ++ t/t5550-http-fetch.sh | 10 +++++----- 2 files changed, 7 insertions(+), 5 deletions(-) diff --git a/http.c b/http.c index f3f83d7..c6dc9b7 100644 --- a/http.c +++ b/http.c @@ -494,6 +494,8 @@ struct active_request_slot *get_active_slot(void) curl_easy_setopt(slot->curl, CURLOPT_POSTFIELDS, NULL); curl_easy_setopt(slot->curl, CURLOPT_UPLOAD, 0); curl_easy_setopt(slot->curl, CURLOPT_HTTPGET, 1); + if (http_auth.password) + init_curl_http_auth(slot->curl); return slot; } diff --git a/t/t5550-http-fetch.sh b/t/t5550-http-fetch.sh index 1d9ff1e..b06f817 100755 --- a/t/t5550-http-fetch.sh +++ b/t/t5550-http-fetch.sh @@ -92,21 +92,21 @@ test_expect_success 'http auth can use user/pass in URL' ' expect_askpass none ' -test_expect_failure 'http auth can use just user in URL' ' +test_expect_success 'http auth can use just user in URL' ' >askpass-query && echo user@host >askpass-response && git clone "$HTTPD_URL_USER/auth/repo.git" clone-auth-pass && expect_askpass pass user@host ' -test_expect_failure 'http auth can request both user and pass' ' +test_expect_success 'http auth can request both user and pass' ' >askpass-query && echo user@host >askpass-response && git clone "$HTTPD_URL/auth/repo.git" clone-auth-both && expect_askpass both user@host ' -test_expect_failure 'http auth respects credential helper config' ' +test_expect_success 'http auth respects credential helper config' ' test_config_global credential.helper "!f() { cat >/dev/null echo username=user@host @@ -118,7 +118,7 @@ test_expect_failure 'http auth respects credential helper config' ' expect_askpass none ' -test_expect_failure 'http auth can get username from config' ' +test_expect_success 'http auth can get username from config' ' test_config_global "credential.$HTTPD_URL.username" user@host && >askpass-query && echo user@host >askpass-response && @@ -126,7 +126,7 @@ test_expect_failure 'http auth can get username from config' ' expect_askpass pass user@host ' -test_expect_failure 'configured username does not override URL' ' +test_expect_success 'configured username does not override URL' ' test_config_global "credential.$HTTPD_URL.username" wrong && >askpass-query && echo user@host >askpass-response && -- 1.7.9.6 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH 2/2] fix http auth with multiple curl handles 2012-04-10 9:53 ` [PATCH 2/2] fix http auth " Clemens Buchacher @ 2012-04-12 7:09 ` Jeff King 2012-04-12 22:50 ` Junio C Hamano 0 siblings, 1 reply; 12+ messages in thread From: Jeff King @ 2012-04-12 7:09 UTC (permalink / raw) To: Clemens Buchacher; +Cc: git, Junio C Hamano On Tue, Apr 10, 2012 at 11:53:40AM +0200, Clemens Buchacher wrote: > HTTP authentication is currently handled by get_refs and fetch_ref, but > not by fetch_object, fetch_pack or fetch_alternates. In the > single-threaded case, this is not an issue, since get_refs is always > called first. It recognigzes the 401 and prompts the user for > credentials, which will then be used subsequently. > > If the curl multi interface is used, however, only the multi handle used > by get_refs will have credentials configured. Requests made by other > handles fail with an authentication error. > > Fix this by setting CURLOPT_USERPWD whenever a slot is requested. The reason I didn't like my initial patch was that by calling curl_easy_setopt() for every request, we end up leaking the password buffer once per request. Unfortunately, I don't think there's a way to ask curl whether it has anything in CURLOPT_USERPWD. So we have to overwrite. Recent versions of curl will actually copy the string we give it anyway, so the existing code is already leaking a little bit (but once per process, not once per request). I wish we could get away with just handing curl the data and assuming it would copy, but that code came about in curl 7.17.0, in 2007. According to our #if statements, we handle much older versions of curl, so that is a non-option. I think the best we can do is to put the auth data in a static buffer and feed that to curl. We end up rewriting the auth data into our buffer over and over, but at least we don't re-malloc it. Like this: diff --git a/http.c b/http.c index f3f83d7..374c3bb 100644 --- a/http.c +++ b/http.c @@ -211,12 +211,12 @@ static int http_options(const char *var, const char *value, void *cb) static void init_curl_http_auth(CURL *result) { if (http_auth.username) { - struct strbuf up = STRBUF_INIT; + static struct strbuf up = STRBUF_INIT; credential_fill(&http_auth); + strbuf_reset(&up); strbuf_addf(&up, "%s:%s", http_auth.username, http_auth.password); - curl_easy_setopt(result, CURLOPT_USERPWD, - strbuf_detach(&up, NULL)); + curl_easy_setopt(result, CURLOPT_USERPWD, up.buf); } } By the way, this touches on an area that I noticed while refactoring the http auth code a while ago, but decided not to tackle at the time. We fill in the auth information early, and then never bother to revisit it as URLs change. So for example, if I got a redirect from host A to host B, we would continue to use the credential for host A and host B. Which is maybe convenient, and maybe a security issue. It has been that way since the beginning of git, and nobody has seemed to care. So maybe it is not worth dealing with. But if we did want to, the right way would be to keep several credentials on hand, and match each URL to them as we were about to request it. That would also provide a fix to the problem we are fixing here. I don't know if it is worth doing or not. -Peff ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH 2/2] fix http auth with multiple curl handles 2012-04-12 7:09 ` Jeff King @ 2012-04-12 22:50 ` Junio C Hamano 2012-04-13 6:16 ` Jeff King 0 siblings, 1 reply; 12+ messages in thread From: Junio C Hamano @ 2012-04-12 22:50 UTC (permalink / raw) To: Jeff King; +Cc: Clemens Buchacher, git Jeff King <peff@peff.net> writes: > ... > I think the best we can do is to put the auth data in a static buffer > and feed that to curl. We end up rewriting the auth data into our buffer > over and over, but at least we don't re-malloc it. Like this: > > diff --git a/http.c b/http.c > index f3f83d7..374c3bb 100644 > --- a/http.c > +++ b/http.c > @@ -211,12 +211,12 @@ static int http_options(const char *var, const char *value, void *cb) > static void init_curl_http_auth(CURL *result) > { > if (http_auth.username) { > - struct strbuf up = STRBUF_INIT; > + static struct strbuf up = STRBUF_INIT; > credential_fill(&http_auth); > + strbuf_reset(&up); > strbuf_addf(&up, "%s:%s", > http_auth.username, http_auth.password); > - curl_easy_setopt(result, CURLOPT_USERPWD, > - strbuf_detach(&up, NULL)); > + curl_easy_setopt(result, CURLOPT_USERPWD, up.buf); > } > } Yeah, that's sad but I agree that is probably the best we could do. Do you want me to squash it in? > By the way, this touches on an area that I noticed while refactoring the > http auth code a while ago, but decided not to tackle at the time. We > fill in the auth information early, and then never bother to revisit it > as URLs change. So for example, if I got a redirect from host A to host > B, we would continue to use the credential for host A and host B. Which > is maybe convenient, and maybe a security issue. Good point. Do we follow redirects, though? ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/2] fix http auth with multiple curl handles 2012-04-12 22:50 ` Junio C Hamano @ 2012-04-13 6:16 ` Jeff King 2012-04-13 6:18 ` [PATCH 1/2] http: clean up leak in init_curl_http_auth Jeff King 2012-04-13 6:19 ` [PATCH 2/2] http: use newer curl options for setting credentials Jeff King 0 siblings, 2 replies; 12+ messages in thread From: Jeff King @ 2012-04-13 6:16 UTC (permalink / raw) To: Junio C Hamano; +Cc: Clemens Buchacher, git On Thu, Apr 12, 2012 at 03:50:08PM -0700, Junio C Hamano wrote: > > diff --git a/http.c b/http.c > > index f3f83d7..374c3bb 100644 > > --- a/http.c > > +++ b/http.c > > @@ -211,12 +211,12 @@ static int http_options(const char *var, const char *value, void *cb) > > static void init_curl_http_auth(CURL *result) > > { > > if (http_auth.username) { > > - struct strbuf up = STRBUF_INIT; > > + static struct strbuf up = STRBUF_INIT; > > credential_fill(&http_auth); > > + strbuf_reset(&up); > > strbuf_addf(&up, "%s:%s", > > http_auth.username, http_auth.password); > > - curl_easy_setopt(result, CURLOPT_USERPWD, > > - strbuf_detach(&up, NULL)); > > + curl_easy_setopt(result, CURLOPT_USERPWD, up.buf); > > } > > } > > Yeah, that's sad but I agree that is probably the best we could do. Do > you want me to squash it in? We can use an #if-macro and only do the static thing for older versions of curl (newer versions copy for us). But since we'd have to carry the old code anyway, it doesn't clean up much. However, since there is an even newer interface for giving curl the credential, it might be worth conditionally including that. I'd rather not squash this bit. It's sufficiently subtle that I'd rather do it on top as a separate patch. Plus I gotta keep up my shortlog stats. ;) So here are the two patches I think we should apply on top of what you have in ct/http-multi-curl-auth. [1/2]: http: clean up leak in init_curl_http_auth [2/2]: http: use newer curl options for setting credentials > > By the way, this touches on an area that I noticed while refactoring the > > http auth code a while ago, but decided not to tackle at the time. We > > fill in the auth information early, and then never bother to revisit it > > as URLs change. So for example, if I got a redirect from host A to host > > B, we would continue to use the credential for host A and host B. Which > > is maybe convenient, and maybe a security issue. > > Good point. Do we follow redirects, though? Curl follows them for us, since we set CURLOPT_FOLLOWLOCATION. However, it does say this under CURLOPT_USERPWD (which I somehow missed the last time I looked into this issue): When using HTTP and CURLOPT_FOLLOWLOCATION, libcurl might perform several requests to possibly different hosts. libcurl will only send this user and password information to hosts using the initial host name (unless CURLOPT_UNRESTRICTED_AUTH is set), so if libcurl follows locations to other hosts it will not send the user and password to those. This is enforced to prevent accidental information leakage. So it looks like we are already doing the safe thing (unless we redirect ourselves outside of curl, but I don't think we ever do so). We won't properly retry auth if we get a 401 on a redirect (we never retry auth if we already had both a username and password; we complain and exit). But nobody has been complaining about that, so I guess it is a non-issue. -Peff ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 1/2] http: clean up leak in init_curl_http_auth 2012-04-13 6:16 ` Jeff King @ 2012-04-13 6:18 ` Jeff King 2012-04-13 6:19 ` [PATCH 2/2] http: use newer curl options for setting credentials Jeff King 1 sibling, 0 replies; 12+ messages in thread From: Jeff King @ 2012-04-13 6:18 UTC (permalink / raw) To: Junio C Hamano; +Cc: Clemens Buchacher, git When we have a credential to give to curl, we must copy it into a "user:pass" buffer and then hand the buffer to curl. Old versions of curl did not copy the buffer, and we were expected to keep it valid. Newer versions of curl will copy the buffer. Our solution was to use a strbuf and detach it, giving ownership of the resulting buffer to curl. However, this meant that we were leaking the buffer on newer versions of curl, since curl was just copying it and throwing away the string we passed. Furthermore, when we replaced a credential (e.g., because our original one was rejected), we were also leaking on both old and new versions of curl. This got even worse in the last patch, which started replacing the credential (and thus leaking) on every http request. Instead, let's use a static buffer to make the ownership more clear and less leaky. We already keep a static "struct credential", so we are only handling a single credential at a time, anyway. Signed-off-by: Jeff King <peff@peff.net> --- The "old" version was prior to 7.17.0, from 2007. That is probably still new enough to care about. But at some point we should probably make a sweep through http.c and get rid of conditional blocks for really old cruft. http.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/http.c b/http.c index c6dc9b7..eaf7f40 100644 --- a/http.c +++ b/http.c @@ -211,12 +211,12 @@ static int http_options(const char *var, const char *value, void *cb) static void init_curl_http_auth(CURL *result) { if (http_auth.username) { - struct strbuf up = STRBUF_INIT; + static struct strbuf up = STRBUF_INIT; credential_fill(&http_auth); + strbuf_reset(&up); strbuf_addf(&up, "%s:%s", http_auth.username, http_auth.password); - curl_easy_setopt(result, CURLOPT_USERPWD, - strbuf_detach(&up, NULL)); + curl_easy_setopt(result, CURLOPT_USERPWD, up.buf); } } -- 1.7.9.6.6.g6b3b56 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH 2/2] http: use newer curl options for setting credentials 2012-04-13 6:16 ` Jeff King 2012-04-13 6:18 ` [PATCH 1/2] http: clean up leak in init_curl_http_auth Jeff King @ 2012-04-13 6:19 ` Jeff King 1 sibling, 0 replies; 12+ messages in thread From: Jeff King @ 2012-04-13 6:19 UTC (permalink / raw) To: Junio C Hamano; +Cc: Clemens Buchacher, git We give the username and password to curl by sticking them in a buffer of the form "user:pass" and handing the result to CURLOPT_USERPWD. Since curl 7.19.1, there is a split mechanism, where you can specify each element individually. This has the advantage that a username can contain a ":" character. It also is less code for us, since we can hand our strings over to curl directly. And since curl 7.17.0 and higher promise to copy the strings for us, we we don't even have to worry about memory ownership issues. Unfortunately, we have to keep the ugly code for old curl around, but as it is now nicely #if'd out, we can easily get rid of it when we decide that 7.19.1 is "old enough". Signed-off-by: Jeff King <peff@peff.net> --- For reference, 7.19.1 is from late 2008. http.c | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/http.c b/http.c index eaf7f40..2ec3789 100644 --- a/http.c +++ b/http.c @@ -210,14 +210,23 @@ static int http_options(const char *var, const char *value, void *cb) static void init_curl_http_auth(CURL *result) { - if (http_auth.username) { + if (!http_auth.username) + return; + + credential_fill(&http_auth); + +#if LIBCURL_VERSION_NUM >= 0x071301 + curl_easy_setopt(result, CURLOPT_USERNAME, http_auth.username); + curl_easy_setopt(result, CURLOPT_PASSWORD, http_auth.password); +#else + { static struct strbuf up = STRBUF_INIT; - credential_fill(&http_auth); strbuf_reset(&up); strbuf_addf(&up, "%s:%s", http_auth.username, http_auth.password); curl_easy_setopt(result, CURLOPT_USERPWD, up.buf); } +#endif } static int has_cert_password(void) -- 1.7.9.6.6.g6b3b56 ^ permalink raw reply related [flat|nested] 12+ messages in thread
end of thread, other threads:[~2012-04-13 6:19 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2012-04-01 18:48 Cannot clone the git repository shared over http with authorization Artur R. Czechowski 2012-04-01 19:45 ` Clemens Buchacher 2012-04-01 20:53 ` Clemens Buchacher 2012-04-02 8:31 ` Jeff King 2012-04-10 9:53 ` Clemens Buchacher 2012-04-10 9:53 ` [PATCH 1/2] http auth fails with multiple curl handles Clemens Buchacher 2012-04-10 9:53 ` [PATCH 2/2] fix http auth " Clemens Buchacher 2012-04-12 7:09 ` Jeff King 2012-04-12 22:50 ` Junio C Hamano 2012-04-13 6:16 ` Jeff King 2012-04-13 6:18 ` [PATCH 1/2] http: clean up leak in init_curl_http_auth Jeff King 2012-04-13 6:19 ` [PATCH 2/2] http: use newer curl options for setting credentials Jeff King
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.