Re: [libmicrohttpd] Re: HTTP Digest Auth done

[Carlos Henrique Júnior]

I'm not really following this thread, but per last update I have a
concern: we must always remember that you can auth only by the
password (leaving username blank).

Carlos Júnior <address@hidden>
www.milk-it.net
+55 31 8763-5606
+55 31 3227-1009



On Thu, Aug 19, 2010 at 8:46 AM, Amr Ali <address@hidden> wrote:
> -----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-----
>
>