From 66c8448543432308e8fce5e3e04076e875410f67 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Mon, 18 Jul 2011 03:48:51 -0400 Subject: url: decode buffers that are not NUL-terminated The url_decode function needs only minor tweaks to handle arbitrary buffers. Let's do those tweaks, which cleans up an unreadable mess of temporary strings in http.c. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano diff --git a/http.c b/http.c index a1ea3db..c93716c 100644 --- a/http.c +++ b/http.c @@ -307,8 +307,7 @@ static CURL *get_curl_handle(void) static void http_auth_init(const char *url) { - char *at, *colon, *cp, *slash, *decoded; - int len; + char *at, *colon, *cp, *slash; cp = strstr(url, "://"); if (!cp) @@ -328,29 +327,11 @@ static void http_auth_init(const char *url) return; /* No credentials */ if (!colon || at <= colon) { /* Only username */ - len = at - cp; - user_name = xmalloc(len + 1); - memcpy(user_name, cp, len); - user_name[len] = '\0'; - decoded = url_decode(user_name); - free(user_name); - user_name = decoded; + user_name = url_decode_mem(cp, at - cp); user_pass = NULL; } else { - len = colon - cp; - user_name = xmalloc(len + 1); - memcpy(user_name, cp, len); - user_name[len] = '\0'; - decoded = url_decode(user_name); - free(user_name); - user_name = decoded; - len = at - (colon + 1); - user_pass = xmalloc(len + 1); - memcpy(user_pass, colon + 1, len); - user_pass[len] = '\0'; - decoded = url_decode(user_pass); - free(user_pass); - user_pass = decoded; + user_name = url_decode_mem(cp, colon - cp); + user_pass = url_decode_mem(colon + 1, at - (colon + 1)); } } diff --git a/url.c b/url.c index 3e06fd3..389d9da 100644 --- a/url.c +++ b/url.c @@ -68,18 +68,20 @@ static int url_decode_char(const char *q) return val; } -static char *url_decode_internal(const char **query, const char *stop_at, - struct strbuf *out, int decode_plus) +static char *url_decode_internal(const char **query, int len, + const char *stop_at, struct strbuf *out, + int decode_plus) { const char *q = *query; - do { + while (len) { unsigned char c = *q; if (!c) break; if (stop_at && strchr(stop_at, c)) { q++; + len--; break; } @@ -88,6 +90,7 @@ static char *url_decode_internal(const char **query, const char *stop_at, if (0 <= val) { strbuf_addch(out, val); q += 3; + len -= 3; continue; } } @@ -97,34 +100,41 @@ static char *url_decode_internal(const char **query, const char *stop_at, else strbuf_addch(out, c); q++; - } while (1); + len--; + } *query = q; return strbuf_detach(out, NULL); } char *url_decode(const char *url) { + return url_decode_mem(url, strlen(url)); +} + +char *url_decode_mem(const char *url, int len) +{ struct strbuf out = STRBUF_INIT; - const char *colon = strchr(url, ':'); + const char *colon = memchr(url, ':', len); /* Skip protocol part if present */ if (colon && url < colon) { strbuf_add(&out, url, colon - url); + len -= colon - url; url = colon; } - return url_decode_internal(&url, NULL, &out, 0); + return url_decode_internal(&url, len, NULL, &out, 0); } char *url_decode_parameter_name(const char **query) { struct strbuf out = STRBUF_INIT; - return url_decode_internal(query, "&=", &out, 1); + return url_decode_internal(query, -1, "&=", &out, 1); } char *url_decode_parameter_value(const char **query) { struct strbuf out = STRBUF_INIT; - return url_decode_internal(query, "&", &out, 1); + return url_decode_internal(query, -1, "&", &out, 1); } void end_url_with_slash(struct strbuf *buf, const char *url) diff --git a/url.h b/url.h index 7100e32..abdaf6f 100644 --- a/url.h +++ b/url.h @@ -4,6 +4,7 @@ extern int is_url(const char *url); extern int is_urlschemechar(int first_flag, int ch); extern char *url_decode(const char *url); +extern char *url_decode_mem(const char *url, int len); extern char *url_decode_parameter_name(const char **query); extern char *url_decode_parameter_value(const char **query); -- cgit v0.10.2-6-g49f6 From 5232586c7985e6a420ee741e19e7fd6d040d43f6 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Mon, 18 Jul 2011 03:49:12 -0400 Subject: improve httpd auth tests These just checked that we could clone a repository when the username and password were given in the URL; we should also check that git will prompt when no or partial credentials are given. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano diff --git a/t/lib-httpd.sh b/t/lib-httpd.sh index b8996a3..f7dc078 100644 --- a/t/lib-httpd.sh +++ b/t/lib-httpd.sh @@ -81,8 +81,7 @@ prepare_httpd() { if test -n "$LIB_HTTPD_SSL" then - HTTPD_URL=https://127.0.0.1:$LIB_HTTPD_PORT - AUTH_HTTPD_URL=https://user%40host:user%40host@127.0.0.1:$LIB_HTTPD_PORT + HTTPD_PROTO=https RANDFILE_PATH="$HTTPD_ROOT_PATH"/.rnd openssl req \ -config "$TEST_PATH/ssl.cnf" \ @@ -93,9 +92,12 @@ prepare_httpd() { export GIT_SSL_NO_VERIFY HTTPD_PARA="$HTTPD_PARA -DSSL" else - HTTPD_URL=http://127.0.0.1:$LIB_HTTPD_PORT - AUTH_HTTPD_URL=http://user%40host:user%40host@127.0.0.1:$LIB_HTTPD_PORT + HTTPD_PROTO=http fi + HTTPD_DEST=127.0.0.1:$LIB_HTTPD_PORT + HTTPD_URL=$HTTPD_PROTO://$HTTPD_DEST + HTTPD_URL_USER=$HTTPD_PROTO://user%40host@$HTTPD_DEST + HTTPD_URL_USER_PASS=$HTTPD_PROTO://user%40host:user%40host@$HTTPD_DEST if test -n "$LIB_HTTPD_DAV" -o -n "$LIB_HTTPD_SVN" then diff --git a/t/t5550-http-fetch.sh b/t/t5550-http-fetch.sh index a1883ca..ed4db09 100755 --- a/t/t5550-http-fetch.sh +++ b/t/t5550-http-fetch.sh @@ -35,11 +35,54 @@ test_expect_success 'clone http repository' ' test_cmp file clone/file ' -test_expect_success 'clone http repository with authentication' ' +test_expect_success 'create password-protected repository' ' mkdir "$HTTPD_DOCUMENT_ROOT_PATH/auth/" && - cp -Rf "$HTTPD_DOCUMENT_ROOT_PATH/repo.git" "$HTTPD_DOCUMENT_ROOT_PATH/auth/repo.git" && - git clone $AUTH_HTTPD_URL/auth/repo.git clone-auth && - test_cmp file clone-auth/file + cp -Rf "$HTTPD_DOCUMENT_ROOT_PATH/repo.git" \ + "$HTTPD_DOCUMENT_ROOT_PATH/auth/repo.git" +' + +test_expect_success 'setup askpass helpers' ' + cat >askpass <<-EOF && + #!/bin/sh + echo >>"$PWD/askpass-query" "askpass: \$*" && + cat "$PWD/askpass-response" + EOF + chmod +x askpass && + GIT_ASKPASS="$PWD/askpass" && + export GIT_ASKPASS && + >askpass-expect-none && + echo "askpass: Password: " >askpass-expect-pass && + { echo "askpass: Username: " && + cat askpass-expect-pass + } >askpass-expect-both +' + +test_expect_success 'cloning password-protected repository can fail' ' + >askpass-query && + echo wrong >askpass-response && + test_must_fail git clone "$HTTPD_URL/auth/repo.git" clone-auth-fail && + test_cmp askpass-expect-both askpass-query +' + +test_expect_success 'http auth can use user/pass in URL' ' + >askpass-query && + echo wrong >askpass-reponse && + git clone "$HTTPD_URL_USER_PASS/auth/repo.git" clone-auth-none && + test_cmp askpass-expect-none askpass-query +' + +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 && + test_cmp askpass-expect-pass askpass-query +' + +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 && + test_cmp askpass-expect-both askpass-query ' test_expect_success 'fetch changes via http' ' -- cgit v0.10.2-6-g49f6 From 28d0c1017a10a93ce165a2d4e9fb6a691a933bd3 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Mon, 18 Jul 2011 03:49:56 -0400 Subject: remote-curl: don't retry auth failures with dumb protocol When fetching an http URL, we first try fetching info/refs with an extra "service" parameter. This will work for a smart-http server, or a dumb server which ignores extra parameters when fetching files. If that fails, we retry without the extra parameter to remain compatible with dumb servers which didn't like our first request. If the server returned a "401 Unauthorized", indicating that the credentials we provided were not good, there is not much point in retrying. With the current code, we just waste an extra round trip to the HTTP server before failing. But as the http code becomes smarter about throwing away rejected credentials and re-prompting the user for new ones (which it will later in this series), this will become more confusing. At some point we will stop asking for credentials to retry smart http, and will be asking for credentials to retry dumb http. So now we're not only wasting an extra HTTP round trip for something that is unlikely to work, but we're making the user re-type their password for it. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano diff --git a/remote-curl.c b/remote-curl.c index faaeda4..6c24ab1 100644 --- a/remote-curl.c +++ b/remote-curl.c @@ -115,7 +115,7 @@ static struct discovery* discover_refs(const char *service) http_ret = http_get_strbuf(refs_url, &buffer, HTTP_NO_CACHE); /* try again with "plain" url (no ? or & appended) */ - if (http_ret != HTTP_OK) { + if (http_ret != HTTP_OK && http_ret != HTTP_NOAUTH) { free(refs_url); strbuf_reset(&buffer); -- cgit v0.10.2-6-g49f6 From 8d677edc4fa3fd1fe12b49bf279aaad5be89b81c Mon Sep 17 00:00:00 2001 From: Jeff King Date: Mon, 18 Jul 2011 03:50:14 -0400 Subject: http: retry authentication failures for all http requests Commit 42653c0 (Prompt for a username when an HTTP request 401s, 2010-04-01) changed http_get_strbuf to prompt for credentials when we receive a 401, but didn't touch http_get_file. The latter is called only for dumb http; while it's usually the case that people don't use authentication on top of dumb http, there is no reason not to allow both types of requests to use this feature. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano diff --git a/http.c b/http.c index c93716c..89e3cf4 100644 --- a/http.c +++ b/http.c @@ -846,13 +846,18 @@ static int http_request(const char *url, void *result, int target, int options) return ret; } +static int http_request_reauth(const char *url, void *result, int target, + int options) +{ + int ret = http_request(url, result, target, options); + if (ret != HTTP_REAUTH) + return ret; + return http_request(url, result, target, options); +} + int http_get_strbuf(const char *url, struct strbuf *result, int options) { - int http_ret = http_request(url, result, HTTP_REQUEST_STRBUF, options); - if (http_ret == HTTP_REAUTH) { - http_ret = http_request(url, result, HTTP_REQUEST_STRBUF, options); - } - return http_ret; + return http_request_reauth(url, result, HTTP_REQUEST_STRBUF, options); } /* @@ -875,7 +880,7 @@ static int http_get_file(const char *url, const char *filename, int options) goto cleanup; } - ret = http_request(url, result, HTTP_REQUEST_FILE, options); + ret = http_request_reauth(url, result, HTTP_REQUEST_FILE, options); fclose(result); if ((ret == HTTP_OK) && move_temp_to_file(tmpfile.buf, filename)) -- cgit v0.10.2-6-g49f6 From 070b4dd5899927cb399b8ae9d75cad1fce537429 Mon Sep 17 00:00:00 2001 From: Michael J Gruber Date: Fri, 14 Oct 2011 09:40:39 +0200 Subject: http: use hostname in credential description Until now, a request for an http password looked like: Username: Password: Now it will look like: Username for 'example.com': Password for 'example.com': Picked-from: Jeff King Signed-off-by: Michael J Gruber Signed-off-by: Junio C Hamano diff --git a/http.c b/http.c index 89e3cf4..7ae0c2a 100644 --- a/http.c +++ b/http.c @@ -42,7 +42,7 @@ static long curl_low_speed_time = -1; static int curl_ftp_no_epsv; static const char *curl_http_proxy; static const char *curl_cookie_file; -static char *user_name, *user_pass; +static char *user_name, *user_pass, *description; static const char *user_agent; #if LIBCURL_VERSION_NUM >= 0x071700 @@ -139,6 +139,27 @@ static void process_curl_messages(void) } #endif +static char *git_getpass_with_description(const char *what, const char *desc) +{ + struct strbuf prompt = STRBUF_INIT; + char *r; + + if (desc) + strbuf_addf(&prompt, "%s for '%s': ", what, desc); + else + strbuf_addf(&prompt, "%s: ", what); + /* + * NEEDSWORK: for usernames, we should do something less magical that + * actually echoes the characters. However, we need to read from + * /dev/tty and not stdio, which is not portable (but getpass will do + * it for us). http.c uses the same workaround. + */ + r = git_getpass(prompt.buf); + + strbuf_release(&prompt); + return xstrdup(r); +} + static int http_options(const char *var, const char *value, void *cb) { if (!strcmp("http.sslverify", var)) { @@ -214,7 +235,7 @@ static void init_curl_http_auth(CURL *result) if (user_name) { struct strbuf up = STRBUF_INIT; if (!user_pass) - user_pass = xstrdup(git_getpass("Password: ")); + user_pass = xstrdup(git_getpass_with_description("Password", description)); strbuf_addf(&up, "%s:%s", user_name, user_pass); curl_easy_setopt(result, CURLOPT_USERPWD, strbuf_detach(&up, NULL)); @@ -229,7 +250,7 @@ static int has_cert_password(void) return 0; /* Only prompt the user once. */ ssl_cert_password_required = -1; - ssl_cert_password = git_getpass("Certificate Password: "); + ssl_cert_password = git_getpass_with_description("Certificate Password", description); if (ssl_cert_password != NULL) { ssl_cert_password = xstrdup(ssl_cert_password); return 1; @@ -307,7 +328,7 @@ static CURL *get_curl_handle(void) static void http_auth_init(const char *url) { - char *at, *colon, *cp, *slash; + const char *at, *colon, *cp, *slash, *host; cp = strstr(url, "://"); if (!cp) @@ -323,16 +344,22 @@ static void http_auth_init(const char *url) at = strchr(cp, '@'); colon = strchr(cp, ':'); slash = strchrnul(cp, '/'); - if (!at || slash <= at) - return; /* No credentials */ - if (!colon || at <= colon) { + if (!at || slash <= at) { + /* No credentials, but we may have to ask for some later */ + host = cp; + } + else if (!colon || at <= colon) { /* Only username */ user_name = url_decode_mem(cp, at - cp); user_pass = NULL; + host = at + 1; } else { user_name = url_decode_mem(cp, colon - cp); user_pass = url_decode_mem(colon + 1, at - (colon + 1)); + host = at + 1; } + + description = url_decode_mem(host, slash - host); } static void set_from_env(const char **var, const char *envname) @@ -828,7 +855,7 @@ static int http_request(const char *url, void *result, int target, int options) * but that is non-portable. Using git_getpass() can at least be stubbed * on other platforms with a different implementation if/when necessary. */ - user_name = xstrdup(git_getpass("Username: ")); + user_name = xstrdup(git_getpass_with_description("Username", description)); init_curl_http_auth(slot->curl); ret = HTTP_REAUTH; } diff --git a/t/t5550-http-fetch.sh b/t/t5550-http-fetch.sh index ed4db09..d1ab4d0 100755 --- a/t/t5550-http-fetch.sh +++ b/t/t5550-http-fetch.sh @@ -51,8 +51,8 @@ test_expect_success 'setup askpass helpers' ' GIT_ASKPASS="$PWD/askpass" && export GIT_ASKPASS && >askpass-expect-none && - echo "askpass: Password: " >askpass-expect-pass && - { echo "askpass: Username: " && + echo "askpass: Password for '\''$HTTPD_DEST'\'': " >askpass-expect-pass && + { echo "askpass: Username for '\''$HTTPD_DEST'\'': " && cat askpass-expect-pass } >askpass-expect-both ' -- cgit v0.10.2-6-g49f6 From deba49377b717d1e26c342f65c7f5e75a2db8641 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Fri, 14 Oct 2011 09:40:40 +0200 Subject: http_init: accept separate URL parameter The http_init function takes a "struct remote". Part of its initialization procedure is to look at the remote's url and grab some auth-related parameters. However, using the url included in the remote is: - wrong; the remote-curl helper may have a separate, unrelated URL (e.g., from remote.*.pushurl). Looking at the remote's configured url is incorrect. - incomplete; http-fetch doesn't have a remote, so passes NULL. So http_init never gets to see the URL we are actually going to use. - cumbersome; http-push has a similar problem to http-fetch, but actually builds a fake remote just to pass in the URL. Instead, let's just add a separate URL parameter to http_init, and all three callsites can pass in the appropriate information. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano diff --git a/http-fetch.c b/http-fetch.c index 3af4c71b..e341872 100644 --- a/http-fetch.c +++ b/http-fetch.c @@ -63,7 +63,7 @@ int main(int argc, const char **argv) git_config(git_default_config, NULL); - http_init(NULL); + http_init(NULL, url); walker = get_http_walker(url); walker->get_tree = get_tree; walker->get_history = get_history; diff --git a/http-push.c b/http-push.c index 6e8f6d0..ecbfae5 100644 --- a/http-push.c +++ b/http-push.c @@ -1747,7 +1747,6 @@ int main(int argc, char **argv) int i; int new_refs; struct ref *ref, *local_refs; - struct remote *remote; git_extract_argv0_path(argv[0]); @@ -1821,14 +1820,7 @@ int main(int argc, char **argv) memset(remote_dir_exists, -1, 256); - /* - * Create a minimum remote by hand to give to http_init(), - * primarily to allow it to look at the URL. - */ - remote = xcalloc(sizeof(*remote), 1); - ALLOC_GROW(remote->url, remote->url_nr + 1, remote->url_alloc); - remote->url[remote->url_nr++] = repo->url; - http_init(remote); + http_init(NULL, repo->url); #ifdef USE_CURL_MULTI is_running_queue = 0; diff --git a/http.c b/http.c index 7ae0c2a..00a8553 100644 --- a/http.c +++ b/http.c @@ -369,7 +369,7 @@ static void set_from_env(const char **var, const char *envname) *var = val; } -void http_init(struct remote *remote) +void http_init(struct remote *remote, const char *url) { char *low_speed_limit; char *low_speed_time; @@ -433,11 +433,11 @@ void http_init(struct remote *remote) if (getenv("GIT_CURL_FTP_NO_EPSV")) curl_ftp_no_epsv = 1; - if (remote && remote->url && remote->url[0]) { - http_auth_init(remote->url[0]); + if (url) { + http_auth_init(url); if (!ssl_cert_password_required && getenv("GIT_SSL_CERT_PASSWORD_PROTECTED") && - !prefixcmp(remote->url[0], "https://")) + !prefixcmp(url, "https://")) ssl_cert_password_required = 1; } diff --git a/http.h b/http.h index 0bf8592..3c332a9 100644 --- a/http.h +++ b/http.h @@ -86,7 +86,7 @@ extern void add_fill_function(void *data, int (*fill)(void *)); extern void step_active_slots(void); #endif -extern void http_init(struct remote *remote); +extern void http_init(struct remote *remote, const char *url); extern void http_cleanup(void); extern int data_received; diff --git a/remote-curl.c b/remote-curl.c index 6c24ab1..d4d0910 100644 --- a/remote-curl.c +++ b/remote-curl.c @@ -850,7 +850,7 @@ int main(int argc, const char **argv) url = strbuf_detach(&buf, NULL); - http_init(remote); + http_init(remote, url); do { if (strbuf_getline(&buf, stdin, '\n') == EOF) -- cgit v0.10.2-6-g49f6