Skip to content

Commit

Permalink
http: use credential API to get passwords
Browse files Browse the repository at this point in the history 
This patch converts the http code to use the new credential
API, both for http authentication as well as for getting
certificate passwords.

Most of the code change is simply variable naming (the
passwords are now contained inside the credential struct)
or deletion of obsolete code (the credential code handles
URL parsing and prompting for us).

The behavior should be the same, with one exception: the
credential code will prompt with a description based on the
credential components. Therefore, the old prompt of:

  Username for 'example.com':
  Password for 'example.com':

now looks like:

  Username for 'https://example.com/repo.git':
  Password for 'https://user@example.com/repo.git':

Note that we include more information in each line,
specifically:

  1. We now include the protocol. While more noisy, this is
     an important part of knowing what you are accessing
     (especially if you care about http vs https).

  2. We include the username in the password prompt. This is
     not a big deal when you have just been prompted for it,
     but the username may also come from the remote's URL
     (and after future patches, from configuration or
     credential helpers).  In that case, it's a nice
     reminder of the user for which you're giving the
     password.

  3. We include the path component of the URL. In many
     cases, the user won't care about this and it's simply
     noise (i.e., they'll use the same credential for a
     whole site). However, that is part of a larger
     question, which is whether path components should be
     part of credential context, both for prompting and for
     lookup by storage helpers. That issue will be addressed
     as a whole in a future patch.

Similarly, for unlocking certificates, we used to say:

  Certificate Password for 'example.com':

and we now say:

  Password for 'cert:///path/to/certificate':

Showing the path to the client certificate makes more sense,
as that is what you are unlocking, not "example.com".

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
  • Loading branch information
Jeff King authored and Junio C Hamano committed Dec 12, 2011
1 parent d3e847c commit 148bb6a
Show file tree
Hide file tree
Showing 2 changed files with 52 additions and 99 deletions.
113 changes: 25 additions & 88 deletions http.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
#include "sideband.h"
#include "run-command.h"
#include "url.h"
#include "credential.h"

int data_received;
int active_requests;
Expand Down Expand Up @@ -42,7 +43,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, *description;
static struct credential http_auth = CREDENTIAL_INIT;
static const char *user_agent;

#if LIBCURL_VERSION_NUM >= 0x071700
Expand All @@ -53,7 +54,7 @@ static const char *user_agent;
#define CURLOPT_KEYPASSWD CURLOPT_SSLCERTPASSWD
#endif

static char *ssl_cert_password;
static struct credential cert_auth = CREDENTIAL_INIT;
static int ssl_cert_password_required;

static struct curl_slist *pragma_header;
Expand Down Expand Up @@ -139,27 +140,6 @@ 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)) {
Expand Down Expand Up @@ -232,30 +212,26 @@ static int http_options(const char *var, const char *value, void *cb)

static void init_curl_http_auth(CURL *result)
{
if (user_name) {
if (http_auth.username) {
struct strbuf up = STRBUF_INIT;
if (!user_pass)
user_pass = xstrdup(git_getpass_with_description("Password", description));
strbuf_addf(&up, "%s:%s", user_name, user_pass);
credential_fill(&http_auth);
strbuf_addf(&up, "%s:%s",
http_auth.username, http_auth.password);
curl_easy_setopt(result, CURLOPT_USERPWD,
strbuf_detach(&up, NULL));
}
}

static int has_cert_password(void)
{
if (ssl_cert_password != NULL)
return 1;
if (ssl_cert == NULL || ssl_cert_password_required != 1)
return 0;
/* Only prompt the user once. */
ssl_cert_password_required = -1;
ssl_cert_password = git_getpass_with_description("Certificate Password", description);
if (ssl_cert_password != NULL) {
ssl_cert_password = xstrdup(ssl_cert_password);
return 1;
} else
return 0;
if (!cert_auth.password) {
cert_auth.protocol = xstrdup("cert");
cert_auth.path = xstrdup(ssl_cert);
credential_fill(&cert_auth);
}
return 1;
}

static CURL *get_curl_handle(void)
Expand All @@ -282,7 +258,7 @@ static CURL *get_curl_handle(void)
if (ssl_cert != NULL)
curl_easy_setopt(result, CURLOPT_SSLCERT, ssl_cert);
if (has_cert_password())
curl_easy_setopt(result, CURLOPT_KEYPASSWD, ssl_cert_password);
curl_easy_setopt(result, CURLOPT_KEYPASSWD, cert_auth.password);
#if LIBCURL_VERSION_NUM >= 0x070903
if (ssl_key != NULL)
curl_easy_setopt(result, CURLOPT_SSLKEY, ssl_key);
Expand Down Expand Up @@ -324,42 +300,6 @@ static CURL *get_curl_handle(void)
return result;
}

static void http_auth_init(const char *url)
{
const char *at, *colon, *cp, *slash, *host;

cp = strstr(url, "://");
if (!cp)
return;

/*
* Ok, the URL looks like "proto://something". Which one?
* "proto://<user>:<pass>@<host>/...",
* "proto://<user>@<host>/...", or just
* "proto://<host>/..."?
*/
cp += 3;
at = strchr(cp, '@');
colon = strchr(cp, ':');
slash = strchrnul(cp, '/');
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)
{
const char *val = getenv(envname);
Expand Down Expand Up @@ -432,7 +372,7 @@ void http_init(struct remote *remote, const char *url)
curl_ftp_no_epsv = 1;

if (url) {
http_auth_init(url);
credential_from_url(&http_auth, url);
if (!ssl_cert_password_required &&
getenv("GIT_SSL_CERT_PASSWORD_PROTECTED") &&
!prefixcmp(url, "https://"))
Expand Down Expand Up @@ -481,10 +421,10 @@ void http_cleanup(void)
curl_http_proxy = NULL;
}

if (ssl_cert_password != NULL) {
memset(ssl_cert_password, 0, strlen(ssl_cert_password));
free(ssl_cert_password);
ssl_cert_password = NULL;
if (cert_auth.password != NULL) {
memset(cert_auth.password, 0, strlen(cert_auth.password));
free(cert_auth.password);
cert_auth.password = NULL;
}
ssl_cert_password_required = 0;
}
Expand Down Expand Up @@ -836,17 +776,11 @@ static int http_request(const char *url, void *result, int target, int options)
else if (missing_target(&results))
ret = HTTP_MISSING_TARGET;
else if (results.http_code == 401) {
if (user_name && user_pass) {
if (http_auth.username && http_auth.password) {
credential_reject(&http_auth);
ret = HTTP_NOAUTH;
} else {
/*
* git_getpass is needed here because its very likely stdin/stdout are
* pipes to our parent process. So we instead need to use /dev/tty,
* but that is non-portable. Using git_getpass() can at least be stubbed
* on other platforms with a different implementation if/when necessary.
*/
if (!user_name)
user_name = xstrdup(git_getpass_with_description("Username", description));
credential_fill(&http_auth);
init_curl_http_auth(slot->curl);
ret = HTTP_REAUTH;
}
Expand All @@ -866,6 +800,9 @@ static int http_request(const char *url, void *result, int target, int options)
curl_slist_free_all(headers);
strbuf_release(&buf);

if (ret == HTTP_OK)
credential_approve(&http_auth);

return ret;
}

Expand Down
38 changes: 27 additions & 11 deletions t/t5550-http-fetch.sh
View file
Original file line number Diff line number Diff line change
Expand Up @@ -49,40 +49,56 @@ test_expect_success 'setup askpass helpers' '
EOF
chmod +x askpass &&
GIT_ASKPASS="$PWD/askpass" &&
export GIT_ASKPASS &&
>askpass-expect-none &&
echo "askpass: Password for '\''$HTTPD_DEST'\'': " >askpass-expect-pass &&
{ echo "askpass: Username for '\''$HTTPD_DEST'\'': " &&
cat askpass-expect-pass
} >askpass-expect-both
'
export GIT_ASKPASS
'

expect_askpass() {
dest=$HTTPD_DEST/auth/repo.git
{
case "$1" in
none)
;;
pass)
echo "askpass: Password for 'http://$2@$dest': "
;;
both)
echo "askpass: Username for 'http://$dest': "
echo "askpass: Password for 'http://$2@$dest': "
;;
*)
false
;;
esac
} >askpass-expect &&
test_cmp askpass-expect askpass-query
}

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
expect_askpass both wrong
'

test_expect_success 'http auth can use user/pass in URL' '
>askpass-query &&
echo wrong >askpass-response &&
git clone "$HTTPD_URL_USER_PASS/auth/repo.git" clone-auth-none &&
test_cmp askpass-expect-none askpass-query
expect_askpass none
'

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
expect_askpass pass user@host
'

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
expect_askpass both user@host
'

test_expect_success 'fetch changes via http' '
Expand Down

0 comments on commit 148bb6a

Please sign in to comment.