[libmicrohttpd] Re: HTTP Digest Auth done

[Amr Ali]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1



On 08/19/2010 01:36 PM, Christian Grothoff wrote:
> On Thursday 19 August 2010 13:21:07 Amr Ali wrote:
>> On 08/19/2010 10:08 AM, Christian Grothoff wrote:
>>> Hi!
>>>
>>> I have more comments ;-).
>>>
>>> First, I see that you're using "gcry_md_read" and similar functions for
>>> MD5. That's perfect *if* MHD is being build with SSL support.  However,
>>> for installations without SSL support we also don't link against
>>> libgcrypt.  So configure will need to test for libgcrypt and only enable
>>> the code (and add "- lgcrypt" to the LDFLAGS) if libgcrypt was found. 
>>> Also, your diffs should typically not include changes to generated files
>>> (you send the diff for configure).
>>
>> sorry about the diff fart. Check configure.ac and you will see that there
>> are already checks against libgcrypt, so the code will not be compiled
>> unless libgcrypt is found.
> 
> Ok, good.
>  
>>> +/* convert bin to hex */
>>> +static void
>>> +cvthex(char *bin, int len, char *hex)
>>> +{
>>> +   unsigned short i;
>>> +   unsigned int j;
>>> +
>>> +   for (i = 0; i < len; ++i) {
>>>
>>> This is asking for trouble: if len > 65536, your loop never terminates...
>>> Also, using a "short" is expensive here: unsigned int might be easier to
>>> put in a register on some architectures.  So 'unsigned int i'.
>>
>> All valid points, its just throughout the code, len will never ever get
>> anything above 20. But yeah proper coding is never a bad thing :-P
> 
> I figured that, I just usually plan for someone to copy a function like that 
> and use it in a different context -- better to not create any surprise then...
> 
>>> Also 'const char *bin'.
>>
>> Will do.
>>
>>> +   header = MHD_lookup_connection_value(
>>> +                   connection, MHD_HEADER_KIND, "Authorization");
>>>
>>> We have a
>>> #define MHD_HTTP_HEADER_AUTHORIZATION "Authorization"
>>> in microhttpd.h that should be used instead.
>>
>> Yeah I actively tried to find something like that, but I think I got
>> distracted by something else along the way.
>>
>>> +
>>> +   buffer = malloc(len);
>>> +
>>>
>>> Stack-allocate your buffer (char buffer[len]) -- you're always freeing it
>>> in the same function as far as I can tell.  Same issue exists elsewhere
>>> (I suspect you should be able to do without any malloc/callocing).
>>
>> Well, "buffer" is an abstracted copy of the header received from the
>> client, there is no way I could know the length of that beforehand,
> 
> You don't need to:
> 
> size_t len = calculate_it ();
> char buffer[len];
>  
> use(buffer);
> 
> is fine in non-ancient C.
> 
>>> +   strncpy(header, _BASE, strlen(_BASE));
>>> +   strncat(header, _REALM, strlen(_REALM));
>>> +   strncat(header, _QUOTE, 1);
>>> +   strncat(header, realm, strlen(realm));
>>> +   strncat(header, _QUOTE, 1);
>>> +   strncat(header, _COM, 1);
>>> +   strncat(header, _QOP, strlen(_QOP));
>>> +   strncat(header, _COM, 1);
>>> +   strncat(header, _NONCE, strlen(_NONCE));
>>> +   strncat(header, _QUOTE, 1);
>>> +   strncat(header, nonce, strlen(nonce));
>>> +   strncat(header, _QUOTE, 1);
>>> +   strncat(header, _COM, 1);
>>> +   strncat(header, _OPAQUE, strlen(_OPAQUE));
>>>
>>> Too long.  Lots of calls, lots of 'strlen' calculations, in particular on
>>> 'header'.  Use a single call to snprintf.
>>
>> sure will do, I kept it in this form to make it simple and easy to make
>> changes and calculate the lengths on the fly, so yeah I think snprintf
>> will do. Thought I must let you know that snprintf is not standard C in
>> anyway, so other platforms might have support for it.
> 
> I appreciate not introducing new system calls, but we already use snprintf 
> elsewhere so it is not an issue here.  And for those kinds of exotic 
> platforms 
> that don't have snprintf, there is always gnulib.
>  
>>> +/**
>>> + * Authenticate a client with HTTP Digest Auth according to RFC2617
>>> + *
>>> + * @param connection The MHD connection structure
>>> + * @param data Data to be sent if authentication succeeds
>>> + * @param size Size of the data
>>> + * @param method The request method
>>> + * @param url the URL requested
>>> + * @param username The username needs to be authenticated
>>> + * @param password The password used in authentication
>>> + * @param realm The realm presented to the client
>>> + * @param nonce_timeout The amount of time for a nonce to be
>>> + *                         invalid in seconds
>>> + * @param must_free libmicrohttpd should free data when done
>>> + * @param must_copy libmicrohttpd must make a copy of data
>>> + *                         right away, the data maybe released anytime 
>>> after
>>> + *                         this call returns
>>> + * @return MHD_YES on success, MHD_NO if authentication
>>> + *                         fails for any reason.
>>> + */
>>> +int
>>> +MHD_digest_auth(
>>> +           struct MHD_Connection *connection,
>>> +           void *data,
>>> +           size_t size,
>>> +           const char *method,
>>> +           const char *url,
>>> +           const char *username,
>>> +           const char *password,
>>> +           const char *realm,
>>> +           int nonce_timeout,
>>> +           int must_free,
>>> +           int must_copy
>>> +           );
>>>
>>> Here I have many issues.  This API does not easily support multiple user
>>> names AND ties using it down to also essentially using
>>> MHD_create_reaponse_from_data. Not to mention you cannot use this for
>>> PUT/POST operations as is (since you'd want to authenticate way before
>>> queuing a reply). That must all be avoided.  Here is a sketch of what
>>> could be done:
>>>
>>>
>>> // obtain username for connection if authentication was supplied
>>> // otherwise returns NULL
>>> const char *
>>> MHD_digest_auth_get_username (struct MHD_Connection *connection)
>>
>> Great idea. Will do.
>>
>>> // check if the given connection supplied authentication for the
>>> // given username that matches 'password' (typically passing
>>> // username would be redundant since it can be obtained from
>>> // connection, but this could be used to simplify the case where
>>> // there is only one username); also, from a security point of view
>>> // just passing a password doesn't feel right...
>>> // Note that we can get method/url from connection here.
>>> // return MHD_YES if pw matches, MHD_NO if not, -1 if stale
>>> int
>>> MHD_digest_auth_check(struct MHD_Connection *connection,
>>>
>>>             const char *realm,
>>>             
>>>                                    unsigned int nonce_timeout,
>>>                                
>>>                                const char *username,
>>>                                
>>>                                const char *password);
>>
>> Indeed, couldn't agree more. I think also it will be a good idea if I made
>> a structure like ...
>>
>> struct MHD_Digest_Credentials {
>>      char *username;
>>      char *password;
>> };
>>
>> so that the user can allocate as much username/password pairs as he likes
>> and only pass a pointer to the struct with its length, to support multiple
>> usernames that is. what do you think?
> 
> Not sure I like having additional structs like this in the API.  Also, my 
> idea 
> was that the user would avoid creating them in the first place by first doing 
> "get_username", then doing the lookup to find the password for that username 
> and then passing both to auth_check.  Having to box it into a struct (or 
> passing an array of structs) seems like overkill here -- especially since an 
> array of such structs would require an O(n) operation for auth_check whereas 
> the API above would permit a hashing-based O(1) implementation of the 
> username->password lookup.
Right, actually there is no need for this at all, after I reread what you typed
and your comments below, I figured I just didn't have the mental image you have.
Now that I do, things are all dandy :-)
>  
>>> // queue response that signals authentication failure
>>> // 'signal_stale' should be MHD_YES if 'auth_check' returned -1.
>>> int
>>> MHD_queue_auth_fail_response (struct MHD_Connection *connection,
>>>
>>>                                        const char *realm,
>>>                                        
>>>                                    unsigned int nonce_timeout,
>>>                                    int signal_stale);
>>
>> I don't quite understand the application of this function, can you please
>> elaborate more on this one?
> 
> Simple: the "auth_check" would no longer queue a response at all, but we do 
> need the response-queueing code with the right headers set for the realm and 
> nonce.  So "auth_fail_response" would call "MHD_queue_response" with a header 
> asking for authentication.  
> 
> Essentially, I'm taking your one function apart into 3 pieces: first only 
> gets 
> the username (if present), second only checks authentication against a 
> particular username/pass and third only requests authentication. 
> 
> The application logic  using those three would then be something like:
> 
> 1) get username, if NULL, call "fail_response", done.
> 2) got username, call "auth_check", if fail, call "fail_response" (possibly
>     with signal_stale if auth_check returned -1).
> 3) auth-check succeeded: do normal request handling (application
>     logic processes upload, queues response, etc.).
>  
I love it, that actually eliminated O(n) especially that "n" is processor
intensive (ex. requires multiple MD5 and SHA1 hash generations).

I'll make the changes and submit R2 patch.
>>> I hope you agree that this would be better.  Suggestions to make it even
>>> better would of course be welcome...  I might also have more nitpicks
>>> later ;-).
>>
>> Fire away ;-)
> 
> Not yet ;-).
> 
> Happy hacking,
> 
> Christian
>  
>>> Happy hacking!
>>>
>>> Christian
>>>
>>> On Thursday 19 August 2010 03:25:49 Amr Ali wrote:
>>>> On 08/18/2010 10:18 AM, Christian Grothoff wrote:
>>>>> Hi!
>>>>>
>>>>> On Tuesday 17 August 2010 22:00:20 Amr Ali wrote:
>>>>>> Hi Christian,
>>>>>>
>>>>>> I'm finally done with this module, I replaced the idea of an internal
>>>>>> buffer that stores nonces with implementing a timeout mechanism for
>>>>>> each nonce that is actually embedded into the nonce, so no need for
>>>>>> increasing the memory footprint.
>>>>>
>>>>> Nice -- if done right (so that clients cannot easily manipulate the
>>>>> timeout...).
>>>>
>>>> Well there are 2 vetting stages for the validity of the nonce, keep an
>>>> eye for comments inside `is_authenticated()'. ;-)
>>>>
>>>>>> I however made the nonce timeout to be 300 seconds
>>>>>> (which IMNSHO is quote enough), its already made as a macro that you
>>>>>> can override with -DNONCE_TIMEOUT <SECONDS>.
>>>>>
>>>>> Yuck.  How about giving the timeout as an argument in your API?
>>>>
>>>> Fixed, now you will have to supply nonce timeout thorough
>>>> MHD_digest_auth. Example file updated as well to reflect the changes.
>>>>
>>>>>> I made an example C program for it as well, its completely based on
>>>>>> minimal_example.c just changed/deleted a few calls.
>>>>>
>>>>> Always good.  Do you also have a testcase and documentation (TexInfo)
>>>>> for the tutorial/manual?
>>>>
>>>> I don't know if it will ever need unit testing. I think the example will
>>>> demonstrate if it is working or not against any browser. There are of
>>>> course few cases that won't be exactly visible thorough a browser like
>>>> in the case of nonce invalidity and how it the code responds to it. But
>>>> meh, we'll see.
>>>>
>>>>>> As for combining this with MHD, I wanted to discuss how you want this
>>>>>> to be combined. The setup I have right now includes the files
>>>>>> `digestauth.c' and `digestauth.h' in src/daemon/Makefile.am, same goes
>>>>>> for
>>>>>> `digest_auth_example.c'. I can change configure.ac to make it optional
>>>>>> and not enabled by default, so if someone wants this, he/she has to
>>>>>> compile it from source with something like "--enable-digest-auth"?
>>>>>
>>>>> Sounds good, but I suspect the default should be "on" eventually:
>>>>> having -- disable-digest-auth (and maybe also
>>>>> --disable-post-processor) will make sure that only developers for
>>>>> embedded systems where code size is critical will disable it and
>>>>> "normal" packages, like say a Debian package for x86, have these
>>>>> enabled without forcing the maintainer to look up options.
>>>>
>>>> Done, in the attached patch, configure will have --disable-digest-auth
>>>> option, which defaults to 'no'.
>>>>
>>>>>> If you think this is good enough I'll make a patch for a the whole
>>>>>> thing and send it your way. If not, please let me know what you have
>>>>>> in your mind.
>>>>>
>>>>> My mindset is getting to the point where new code needs to come with
>>>>> testcases and at least a little bit of documentation ;-).  Despite that
>>>>> request, I think you should send a first version of your patch now so
>>>>> that I can look over the API itself and give you feedback on that and
>>>>> the code.  That way, the test & documentation won't have to be
>>>>> rewritten if the API needs to be adjusted (like with the -D
>>>>> NONCE_TIMEOUT, which is just a bad hack that can really not stay).
>>>>
>>>> See attached!
>>>>
>>>>> Happy hacking
>>>>>
>>>>> Christian
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.10 (GNU/Linux)

iEYEARECAAYFAkxtGa0ACgkQ2VxGY2VcpoiozACdFHEvwLNAc5kbqzzJBqeqZc+L
2HcAn3dFVFdebKqNLGHKJ3YXV4XsH9ej
=iMC/
-----END PGP SIGNATURE-----