Am 2014-07-15 14:19, schrieb David Woodhouse:
> On Tue, 2014-07-15 at 13:18 +0200, Michael Osipov wrote:
>> Am 2014-07-13 22:22, schrieb David Woodhouse:
>>> On Sun, 2014-07-13 at 11:31 +0200, Michael Osipov wrote:
>>>>
>>>> Please have a look:
>>>> https://github.com/michael-o/curl/commit/b78ad621d45f537dfde745e961427257f1e1fc2d
>>>>
>>>> Work is based on top of your patches.
>>>
>>> That really wants splitting into individual patches to make it readable.
>>
>> David,
>>
>> I have split the patch apart and added some more bugfixes I did not
>> notice before.
>>
>> Please have a look again:
>> https://github.com/michael-o/curl/compare/a6bf4636e4...1047baf0e3
>>
>> I'll test that by the end of the week and make a complete patch proposal
>> if everything is fine.
>
>> Michael Osipov (7):
>> Added missing ifdef to Curl_http_done if GSS-API or SSPI is not available
>
> I've merged that fix into the patch which introduced that bug now; thanks.
>
>> Add macros for the most common GSS-API mechs and pass them to
>
> That commit subject is truncated (you can't wrap lines there). And I
> don't like the patch either. I think this wants to be an enum, as
> discussed. That way we can end up presenting the same API for our GSSAPI
> and SSPI implementations, and the code which *uses* them can be the
> same.
>
>> Remove checkprefix("GSS-Negotiate")
>
> OK... but you're about to add half of this back again to handle
> 'WWW-Authenticate: Kerberos'. You'll need the 'protocol' member of
> negotiatedata back again then, and the 'gss' member becomes 'spnego',
> right? So perhaps it makes sense to remove GSS-Negotiate and add
> Kerberos in the *same* patch, rather than in separate patches? Or at
> least do them in consecutive patches.

The issues about would actually require to rename the struct to
something like gssdata from negotiatedata because it is not tied to
SPNEGO anymore -- and yes, in that case an enum would be wise.

E.g.:
#if defined(HAVE_GSSAPI) || defined(USE_WINDOWS_SSPI)
struct gssdata {
   /* When doing GSS-based auth, we first need to send a token
      and then validate the received one. */
   enum { GSS_AUTHNONE, GSS_AUTHRECV, GSS_AUTHSENT } state;
   enum { KERBEROS, SPNEGO, NTLM } mech;
#ifdef HAVE_GSSAPI
   OM_uint32 status;
   gss_ctx_id_t context;
   gss_name_t server_name;
   gss_buffer_desc output_token;
#else
#ifdef USE_WINDOWS_SSPI
   DWORD status;
   CtxtHandle *context;
   CredHandle *credentials;
   char server_name[1024];
   size_t max_token_length;
   BYTE *output_token;
   size_t output_token_length;
#endif
#endif
};
#endif

#ifdef USE_HTTP_NEGOTIATE
   gssdata->state = GSS_AUTHNONE;
   if((authstatus->picked == CURLAUTH_NEGOTIATE) &&
      gssdata->context && !GSS_ERROR(gssdata->status)) {
     auth="Negotiate";
     gssdata->mech = SPNEGO;
     result = Curl_output_gss(conn, proxy);
     if(result)
       return result;
     authstatus->done = TRUE;
     gssdata->state = GSS_AUTHSENT;
   }
   else
#endif

While you are right about "add half of this back again", it not just
like add that enum and you are done. I you must add the define for the
CURLAUTH_, add CURL_VERSION_, register both at the appropriate spots,
add the command line options, etc. That might result in an additional
series of commits. That's why I have abstained from.

For such a change, I'd like to have a commiters consent. This is some
serious work.

>> Improve inline GSS-API naming in code documentation
>
> Not so keen on this one either. I think 'GSSAPI' was better than 'GSS-API'.

The official name is GSS-API, I have that already patched in previous
ticket in the configure.ac. So that is an intentional correction.

About the truncation: I will take care of, it seems like GitHub limits
then to 72 chars.
-------------------------------------------------------------------
List admin: http://cool.haxx.se/list/listinfo/curl-library
Etiquette: http://curl.haxx.se/mail/etiquette.html
Received on 2014-07-15