Re: [PATCH v2] remote-curl: fall back to basic auth if Negotiate fails
- From
- Christopher Schenk <christopher@cschenk.net>
- Date
- Mar 22, 2021, 16:08 UTC
- Message-ID
- <2d995558-1fce-824f-ae65-bcbd3b744cb2@cschenk.net>
- In-Reply-To
- <xmqq35xvpr8q.fsf@gitster.c.googlers.com>
On 2/16/21 11:44 PM, Junio C Hamano wrote:
Show 19 quoted lines
> "Christopher via GitGitGadget" <gitgitgadget@gmail.com> writes: > >> From: Christopher Schenk <christopher@cschenk.net> >> >> When the username and password are supplied in a url like this >> https://myuser:secret@git.exampe/myrepo.git and the server supports the >> negotiate authenticaten method git does not fall back to basic auth and > > s/method git/method, git/; > >> libcurl hardly tries to authenticate with the negotiate method. > > Thanks. > >> Stop using the Negotiate authentication method after the first failure >> because if it fails on the first try it will never succeed. > > Is this patch needed because we are using cURL library incorrectly, > or is it the limitation of the cURL library?
I'm no cURL expert, but in my opinon this is a limitation of the cURL Libary.
Show 7 quoted lines
> >> V1 of this patch somehow did not make it to the mailing list so i will >> try to send this patch again > > The last paragraph does not belong to the commit log message; if > nobody on the list saw the "v1", as far as the project is concerned, > it never happened.
I have adapted the commit message accordingly.
Show 47 quoted lines
>
>> Signed-off-by: Christopher Schenk <christopher@cschenk.net>
>> ---
>
>> diff --git a/http.c b/http.c
>> index 8b23a546afdf..36f113d46c23 100644
>> --- a/http.c
>> +++ b/http.c
>> @@ -1642,6 +1642,14 @@ static int handle_curl_result(struct slot_results *results)
>> return HTTP_MISSING_TARGET;
>> else if (results->http_code == 401) {
>> if (http_auth.username && http_auth.password) {
>> +#ifdef LIBCURL_CAN_HANDLE_AUTH_ANY
>> + if (results->auth_avail & CURLAUTH_GSSNEGOTIATE) {
>> + http_auth_methods &= ~CURLAUTH_GSSNEGOTIATE;
>> + http_auth_methods &= results->auth_avail;
>> + http_auth_methods_restricted = 1;
>> + return HTTP_REAUTH;
>> + }
>> +#endif
>> credential_reject(&http_auth);
>> return HTTP_NOAUTH;
>> } else {
>
> Hmph, is this an extension to what 4dbe6646 (remote-curl: fall back
> to Basic auth if Negotiate fails, 2015-01-08) tried to do? What
> happens on the "else" side after the context of this patch, which
> seems to have come from:
>
> - 4dbe6646 (remote-curl: fall back to Basic auth if Negotiate
> fails, 2015-01-08)
>
> - 840398fe (http: restrict auth methods to what the server
> advertises, 2017-02-22), and
>
> - 40a18fc7 (http: add an "auto" mode for http.emptyauth,
> 2017-02-25),
>
> looks essentially the same as what this patch is adding, and I am
> wondering if there is a room for simplification. It almost looks
> to me that the only difference between "credential fully given" and
> other case is if we "reject" the credential after this patch.
>
> Asking contributors who made these past contributions for input.
>
> Thanks.
> I have simplified the code and sent the patch again.
Thanks.