From: brian m. carlson <sandals@crustytoothpaste.net>
Date: Sat, 03 Jan 2015 17:45:09 GMT
Subject: Re: [PATCH v2] remote-curl: fall back to Basic auth if Negotiate fails
Message-ID: <20150103174509.GA1025060@vauxhall.crustytoothpaste.net>
In-Reply-To: <20150103111922.GB27793@peff.net>

On Sat, Jan 03, 2015 at 06:19:23AM -0500, Jeff King wrote:
>This pattern gets repeated in several places. Now that
>http_passwordless_auth is a global, can we handle it automatically for
>the callers, as below (which, aside from compiling, is completely
>untested by me)?

This looks good (although I haven't tested it).

>Note that this is in a slightly different boat than credential_fill.
>Ideally we would also handle picking up credentials on behalf of the
>callers of get_curl_handle/handle_curl_result. But that may involve
>significant work and/or prompting the user, which we _must_ avoid if we
>do not know if we are going to retry the request (and only the caller
>knows that for sure). However, in the case of http_passwordless_auth, we
>are just setting a flag, so it's OK to do it preemptively.

Right.  We already prompt the user for a username and password in that 
case, so we already have the credentials that we need.

>diff --git a/http.c b/http.c
>index 040f362..2bbcdf1 100644
>--- a/http.c
>+++ b/http.c
>@@ -62,6 +62,8 @@ static const char *user_agent;
>
> static struct credential cert_auth = CREDENTIAL_INIT;
> static int ssl_cert_password_required;
>+/* Should we allow non-password-based authentication (e.g. GSSAPI)? */
>+static int http_passwordless_auth = 1;
>
> static struct curl_slist *pragma_header;
> static struct curl_slist *no_pragma_header;
>@@ -318,7 +320,12 @@ static CURL *get_curl_handle(void)
> 	curl_easy_setopt(result, CURLOPT_NETRC, CURL_NETRC_OPTIONAL);
> #endif
> #ifdef LIBCURL_CAN_HANDLE_AUTH_ANY
>-	curl_easy_setopt(result, CURLOPT_HTTPAUTH, CURLAUTH_ANY);
>+	{
>+		int flags = CURLAUTH_ANY;

I think this needs to be unsigned long or it can cause undefined 
behavior, since libcurl uses unsigned long in the flags.  I'll fix that 
up when I reroll.  I'll need your sign-off since it will essentially be 
your work.

>+		if (!http_passwordless_auth)
>+			flags &= ~CURLAUTH_GSSNEGOTIATE;
>+		curl_easy_setopt(result, CURLOPT_HTTPAUTH, flags);
>+	}
> #endif
>
> 	if (http_proactive_auth)
>@@ -870,6 +877,7 @@ int handle_curl_result(struct slot_results *results)
> 			credential_reject(&http_auth);
> 			return HTTP_NOAUTH;
> 		} else {
>+			http_passwordless_auth = 0;
> 			return HTTP_REAUTH;
> 		}
> 	} else {
>
>
>Note that you could probably drop http_passwordless_auth completely, and
>just keep a:
>
>  static int http_auth_methods = CURLAUTH_ANY;
>
>and then drop CURLAUTH_GSSNEGOTIATE from it instead of setting the
>passwordless_auth flag to 0 (again, it happens in one place, so I don't
>know that it needs an extra layer of abstraction).

This does seem to handle both cases well.  It also has the pleasant 
side effect of being static.
-- 
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | http://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187