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.)

> 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.
>
> 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.

> 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.

> If there would be a check, you could already call Curl_cleanup_negotiate
> here and leaving an additional call with Curl_http_done in case of
> failures. Alternatively, you would call it after all failures.
> Moreover, I fail to see the gss_release_buffer on the input_token when
> the server sent one, is Curl_safefree(input_token.value) enough?

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?

> > 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?

> > negdata->state = GSS_AUTHSENT;
> > }
>
> Curl_output_negotiate():
>
> > gss_release_buffer(&discard_st, &neg_ctx->output_token);
> > neg_ctx->output_token.value = NULL;
> > neg_ctx->output_token.length = 0;
>
> I do not think that release and the assignments are necessary. Release
> ought be enough.

Yes, it shouldn't be necessary. But if a crap version of
gss_release_buffer() doesn't manage to zero them, it doesn't *hurt* for
us to make sure.

-- 
dwmw2

-------------------------------------------------------------------
List admin: http://cool.haxx.se/list/listinfo/curl-library
Etiquette: http://curl.haxx.se/mail/etiquette.html