From 5a9681f46aa9e152f32e76d7ade9d4c11313f99a Mon Sep 17 00:00:00 2001 From: Clemens Buchacher Date: Tue, 10 Apr 2012 11:53:39 +0200 Subject: [PATCH 1/4] http auth fails with multiple curl handles Create a repo with multiple loose objects in order to demonstrate http authentication breakage. Signed-off-by: Clemens Buchacher Signed-off-by: Junio C Hamano --- 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 e5e6b8f64..1d9ff1e0a 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 && From dfa1725a3ec57098637b698ffc2b2e2459acc518 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Tue, 10 Apr 2012 11:53:40 +0200 Subject: [PATCH 2/4] fix http auth with multiple curl handles 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 Signed-off-by: Junio C Hamano --- 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 f3f83d70c..c6dc9b778 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 1d9ff1e0a..b06f817af 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 && From aa0834a04e1d9d3ab81ecd4a4a138233e6e234ed Mon Sep 17 00:00:00 2001 From: Jeff King Date: Fri, 13 Apr 2012 02:18:35 -0400 Subject: [PATCH 3/4] http: clean up leak in init_curl_http_auth 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 Signed-off-by: Junio C Hamano --- http.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/http.c b/http.c index c6dc9b778..eaf7f40d1 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); } } From 6f4c347ca1d3102d77e2dd36b6bc8ab12de6045b Mon Sep 17 00:00:00 2001 From: Jeff King Date: Fri, 13 Apr 2012 02:19:25 -0400 Subject: [PATCH 4/4] http: use newer curl options for setting credentials 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 Signed-off-by: Junio C Hamano --- http.c | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/http.c b/http.c index eaf7f40d1..2ec37891f 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)