Re: [PATCH 1/2] http: allow selection of proxy authentication method
From: Junio C Hamano <hidden>
Date: 2016-06-15 23:07:09
Knut Franke [off-list ref] writes:
CURLAUTH_ANY does not work with proxies which answer unauthenticated requests with a 307 redirect to an error page instead of a 407 listing supported authentication methods. Therefore, allow the authentication method to be set using the environment variable GIT_HTTP_PROXY_AUTHMETHOD or configuration variables http.proxyAuthmethod and remote.<name>.proxyAuthmethod (in analogy to http.proxy and remote.<name>.proxy). The following values are supported: * anyauth (default) * basic * digest * negotiate * ntlm Signed-off-by: Knut Franke <redacted>
Reviewed-by: Junio C Hamano <redacted> Reviewed-by: Eric Sunshine <redacted>
Please add these only when you are doing the final submission, sending the same version reviewed by these people after they said the patch(es) look good. To credit others for helping you to polish your patch, Helped-by: would be more appropriate.
quoted hunk
@@ -305,6 +326,42 @@ static void init_curl_http_auth(CURL *result) #endif } +/* assumes *var is either NULL or free-able */ +static void env_override(const char **var, const char *envname) +{ + const char *val = getenv(envname); + if (val) { + if (*var) + free((void*)*var);
Just free((void *)*var); would be more idiomatic (freeing NULL is not a crime but a norm). Also as you did elsewhere, have a space between void and the asterisk.
+static void init_curl_proxy_auth(CURL *result)
+{
+ env_override(&http_proxy_authmethod, "GIT_HTTP_PROXY_AUTHMETHOD");Shouldn't this also be part of the #if/#endif?
quoted hunk
+ +#if LIBCURL_VERSION_NUM >= 0x070a07 /* CURLOPT_PROXYAUTH and CURLAUTH_ANY */ + if (http_proxy_authmethod) { + int i; + for (i = 0; i < ARRAY_SIZE(proxy_authmethods); i++) { + if (!strcmp(http_proxy_authmethod, proxy_authmethods[i].name)) { + curl_easy_setopt(result, CURLOPT_PROXYAUTH, + proxy_authmethods[i].curlauth_param); + break; + } + } + if (i == ARRAY_SIZE(proxy_authmethods)) { + warning("unsupported proxy authentication method %s: using anyauth", + http_proxy_authmethod); + curl_easy_setopt(result, CURLOPT_PROXYAUTH, CURLAUTH_ANY); + } + } + else + curl_easy_setopt(result, CURLOPT_PROXYAUTH, CURLAUTH_ANY); +#endif +} + static int has_cert_password(void) { if (ssl_cert == NULL || ssl_cert_password_required != 1)@@ -466,9 +523,7 @@ static CURL *get_curl_handle(void) if (curl_http_proxy) { curl_easy_setopt(result, CURLOPT_PROXY, curl_http_proxy); } -#if LIBCURL_VERSION_NUM >= 0x070a07 - curl_easy_setopt(result, CURLOPT_PROXYAUTH, CURLAUTH_ANY); -#endif + init_curl_proxy_auth(result); set_curl_keepalive(result);@@ -509,6 +564,12 @@ void http_init(struct remote *remote, const char *url, int proactive_auth) if (remote && remote->http_proxy) curl_http_proxy = xstrdup(remote->http_proxy); + if (remote && remote->http_proxy_authmethod) { + if (http_proxy_authmethod) + free((void*)http_proxy_authmethod);
Just
free((void *)http_proxy_authmethod);
without NULL-ness check.
But this makes me wonder if env_override() was a good abstraction.
That is, with this helper:
/* existing value in *var must be freeable */
static void var_override(const char **var, char *value)
{
if (value) {
free((void *)(*var));
var = xstrdup(value);
}
}
the beginning of the init_proxy_auth() would become:
static void init_curl_proxy_auth(CURL *result)
{
#if LIBCURL_VERSION_NUM >= 0x070a07 /* CURLOPT_PROXYAUTH and CURLAUTH_ANY */
var_override(&http_proxy_authmethod, getenv("GIT_HTTP_PROXY_AUTHMETHOD"));
...
and this code would be:
if (remote)
var_override(&http_proxy_authmethod, remote->http_proxy_authmethod);
which might be even cleaner.
quoted hunk
+ http_proxy_authmethod = xstrdup(remote->http_proxy_authmethod); + } + pragma_header = curl_slist_append(pragma_header, "Pragma: no-cache"); no_pragma_header = curl_slist_append(no_pragma_header, "Pragma:");@@ -607,6 +668,11 @@ void http_cleanup(void) curl_http_proxy = NULL; } + if (http_proxy_authmethod) { + free((void *)http_proxy_authmethod); + http_proxy_authmethod = NULL; + }
No need for NULL-ness check here, either. Other than the above nits, looks cleanly done. Thanks.