Am 2014-07-13 12:45, schrieb David Woodhouse:
> On Sun, 2014-07-13 at 11:31 +0200, Michael Osipov wrote:
>> Am 2014-07-12 17:58, schrieb David Woodhouse:
>>> [...]
>>>>> So what *do* we want to do on top of the patch set I posted? Just add
>>>>> support for '{Proxy,WWW}-Authenticate: Kerberos'?
>>>>
>>>> I would rather do that after this patch has been tested, approved and
>>>> committed. This is the safest way to implement that improvement on top.
>>>> I don't like to fix two things in one big patch. It ends up in a mess.
>>>
>>> Pfft. It's a set of 7 patches in my tree already; what's wrong with
>>> making it 8? :)
>>
>> That maybe true but in my opinion this can be done this way:
>>
>> rename http_negoatiate.c to http_gssapi.c and your are almost done. A
>> few modified signatures and the very same code does Kerberos and SPNEGO
>> almost for free.
>
> Yes, as I said it's basically just the difference between using the
> SPNEGO OID or the standard one.
>
> There's actually something to be said for ditching http_negotiate_sspi.c
> too, and letting Windows use http_negotiate.c. Let curl_gssapi.c and
> curl_sspi.c both present the *same* interface for a generic
> implementation of "WWW-Authenticate: Negotiate/Kerberos/NTLM" to use.
>
> (Yes, we can use GSSAPI for 'WWW-Authenticate: NTLM' on Linux too, as
> well as invoking the ntlm_auth helper or doing it manually.)

Yes. That should be discussed after this patched has been merge.
Especially with Steve Holme.

>> Now let's get back to the patch. I am half way through your patch. Code
>> looks good with a few glitches. I have improved those and modified a
>> lots of the boiler-plate code. I compiles flawlessly on Linux Mint, make
>> test runs fine too. The non-GSS-API version runs fine.
>> I will test the entire code by the end of the next week at work. So
>> these changes are still pending.
>>
>> Please have a look:
>> https://github.com/michael-o/curl/commit/b78ad621d45f537dfde745e961427257f1e1fc2d
>>
>> Work is based on top of your patches.

Did you have a chance to check the patch actually?

>> There is another issue with the code I'd like you to examine with your C
>> knowledge, mine is rather limited. The entire auth loop workflow is,
>> unfortunately, spread over several files/places which makes it hard to read.
>>
>> Curl_http_input_auth():
>>
>> It receives an auth challenge from the server and passes to
>> Curl_input_negotiate but it does not init the context to NO_CONTEXT but
>> simply passes a NULL pointer.
>
> GSS_C_NO_CONTEXT *is* a NULL pointer.

OK -- I thought it might be to set explicitly to improve readability of
the code.

>> After the first round trip, a mutual token is received but nowhere is
>> saved that the whether auth is actually complete or continue is needed.
>> The enum state does not really help. It does not reflect the looping.
>
> Yeah, but really the only important question is whether the server
> accepts our authentication and lets us have the page. Even if the server
> *does* give us a new WWW-Authenticate: or WWW-Authenticate-Info: header
> with the 200 response, and even if we *do* feed it into the GSSAPI
> context, we're still immediately going to tear down the context and
> throw it away. It's not like with SOCKS where we actually *use* the
> completed context for gss_wrap() etc.
>
> So you can consider this an optimisation. We just don't *care* if it's
> GSS_S_COMPLETE or GSS_S_CONTINUE_NEEDED.

I wouldn't avoid such a fact. I would feed that returned token
explicitly and care about. The entire sense of that mutual auth is gone
when you ignore the token from the server. This is as same as trusting a
cert with is not signed by a trusted CA.

> Yes, that's enough.
>
>> output_auth_headers():
>>
>>> negdata->state = GSS_AUTHNONE;
>>
>> This blind assumes that we always have only one way auth.
>
> Isn't that true in practice?

In most cases. We should annotate that part to make clear that we have
simplified it.

>>> if((authstatus->picked == CURLAUTH_NEGOTIATE) &&
>>> negdata->context && !GSS_ERROR(negdata->status)) {
>>> auth="Negotiate";
>>> result = Curl_output_negotiate(conn, proxy);
>>> if(result)
>>> return result;
>>> authstatus->done = TRUE;
>>
>> This is also wrong, the auth is not complete, CONTINUE_NEEDED is
>> completely ignored. The client must wait for the mutual auth response.
>> There is a multi flag, we should set it to TRUE.
>
> I think I address that above, right?

Yes, you did but I still would not set "authstatus->done = TRUE;". If
you look at the NTLM part, you'll see that this flag is not set -- for a
reason.

Michael
-------------------------------------------------------------------
List admin: http://cool.haxx.se/list/listinfo/curl-library
Etiquette: http://curl.haxx.se/mail/etiquette.html
Received on 2014-07-13