Re: [PATCH 1/2] http: allow selection of proxy authentication method
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Nov 2, 2015, 22:46 UTC
- Message-ID
- <xmqqfv0odnoc.fsf@gitster.mtv.corp.google.com>
- In-Reply-To
- <1446483264-15123-2-git-send-email-k.franke@science-computing.de>
Knut Franke <k.franke@science-computing.de> writes:
Show 16 quoted lines
> 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 <k.franke@science-computing.de>
> Reviewed-by: Junio C Hamano <gitster@pobox.com> > Reviewed-by: Eric Sunshine <sunshine@sunshineco.com>
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.
Show 11 quoted lines
> @@ -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?
Show 43 quoted lines
> +
> +#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.
Show 14 quoted lines
> + 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.