Re: [PATCH] Support various HTTP authentication methods
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Jan 29, 2009, 10:08 UTC
- Message-ID
- <7v3af2h1b0.fsf@gitster.siamese.dyndns.org>
- In-Reply-To
- <1233221532.21518.1.camel@lena.gsc.riken.jp>
Moriyoshi Koizumi <mozo@mozo.jp> writes:
Show 6 quoted lines
> Currently there is no way to specify the preferred authentication > method for the HTTP backend and it always ends up with the CURL's > default > settings. > > This patch enables it if supported by CURL, adding a couple of new
"it" in "enables it" is a bit unclear...
Show 5 quoted lines
> Signed-off-by: Moriyoshi Koizumi <mozo@mozo.jp> > --- > http.c | 105 > ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ > 1 files changed, 105 insertions(+), 0 deletions(-)
Linewrapped and whitespace damaged patch that would not apply.
Show 14 quoted lines
> diff --git a/http.c b/http.c > index ee58799..889135f 100644 > --- a/http.c > +++ b/http.c > @@ -25,6 +25,12 @@ static long curl_low_speed_limit = -1; > static long curl_low_speed_time = -1; > static int curl_ftp_no_epsv = 0; > static const char *curl_http_proxy = NULL; > +#if LIBCURL_VERSION_NUM >= 0x070a06 > +static const char *curl_http_auth = NULL; > +#endif > +#if LIBCURL_VERSION_NUM >= 0x070a07 > +static const char *curl_http_proxy_auth = NULL; > +#endif
I am not a cURL expert, so I'd take your word for these version dependencies.
We do not initialize static scope pointers to "= NULL" nor variables to 0, instead we let BSS take care of that for us. ftp_no_epsv we can see in the context is doing unnecessary initialization that should be fixed.
Show 10 quoted lines
> static struct curl_slist *pragma_header;
>
> @@ -153,11 +159,67 @@ static int http_options(const char *var, const
> char *value, void *cb)
> return git_config_string(&curl_http_proxy, var, value);
> return 0;
> }
> +#if LIBCURL_VERSION_NUM >= 0x070a06
> + if (!strcmp("http.auth", var)) {
> + if (curl_http_auth == NULL)We tend to say "if (!pointer)".
I see you implemented "the first one wins" rule with this test, but I do not think you want that. We first read $HOME/.gitconfig and then repository specific $GIT_DIR/config, so it is often more useful to use "the last one wins" rule.
Show 22 quoted lines
> + return git_config_string(&curl_http_auth, var, value);
> + return 0;
> + }
> +#endif
> +#if LIBCURL_VERSION_NUM >= 0x070a07
> + if (!strcmp("http.proxy_auth", var)) {
> + if (curl_http_proxy_auth == NULL)
> + return git_config_string(&curl_http_proxy_auth, var, value);
> + return 0;
> + }
> +#endif
>
> /* Fall back on the default ones */
> return git_default_config(var, value, cb);
> }
>
> +#if LIBCURL_VERSION_NUM >= 0x070a06
> +static long get_curl_auth_bitmask(const char* auth_method)
> +{
> + char *buf = xmalloc(strlen(auth_method) + 1);
> + const unsigned char *p = (const unsigned char *)auth_method;
> + long mask = CURLAUTH_NONE;Our isspace() is a sane_isspace(), so you do not have to play casting games between signed vs unsigned char.
Show 14 quoted lines
> + for (;;) {
> + char *q = buf;
> + while (*p && isspace(*p))
> + ++p;
> +
> + while (*p && *p != ',')
> + *q++ = tolower(*p++);
> +
> + while (--q >= buf && isspace(*(unsigned char *)q));
> + ++q;
> +
> + *q = '\0';
> +
> + if (strcmp(buf, "basic") == 0)Say !strcmp(buf, "literal") like you did in the configuration parsing part earlier.
Show 16 quoted lines
> + mask |= CURLAUTH_BASIC; > + else if (strcmp(buf, "digest") == 0) > + mask |= CURLAUTH_DIGEST; > + else if (strcmp(buf, "gss") == 0) > + mask |= CURLAUTH_GSSNEGOTIATE; > + else if (strcmp(buf, "ntlm") == 0) > + mask |= CURLAUTH_NTLM; > + else if (strcmp(buf, "any") == 0) > + mask |= CURLAUTH_ANY; > + else if (strcmp(buf, "anysafe") == 0) > + mask |= CURLAUTH_ANYSAFE; > + > + if (!*p) > + break; > + ++p; > + }
You leak "buf" here you forgot to free. The string you can possibly accept is a known set with some maximum length, so you can use a on-stack buf[] and reject any token longer than that maximum, right?
Show 25 quoted lines
> + return mask;
> +}
> +#endif
> +
> static CURL* get_curl_handle(void)
> {
> CURL* result = curl_easy_init();
> @@ -210,6 +272,20 @@ static CURL* get_curl_handle(void)
> if (curl_http_proxy)
> curl_easy_setopt(result, CURLOPT_PROXY, curl_http_proxy);
>
> + if (curl_http_auth) {
> + long n = get_curl_auth_bitmask(curl_http_auth);
> + curl_easy_setopt(result, CURLOPT_HTTPAUTH, n);
> + }
> +
> + if (curl_http_proxy) {
> + curl_easy_setopt(result, CURLOPT_PROXY, curl_http_proxy);
> +
> + if (curl_http_proxy_auth) {
> + long n = get_curl_auth_bitmask(curl_http_proxy_auth);
> + curl_easy_setopt(result, CURLOPT_PROXYAUTH, n);
> + }
> + }
> +This part does not have to be protected with the LIBCURL_VERSION_NUM conditional? I somehow find it unlikely...
Instead of parsing the string every time a curl handle is asked for, how about parsing them once and store the masks in two file scope static longs in http_init() and use that value to easy_setopt() call here?
That way you can free the two strings much early without waiting for http_cleanup(), too, right?