Re: [libmicrohttpd] Re: HTTP Digest Auth done

[Carlos Henrique Júnior]

Good touch, Christian!

But, anyway, my only concern is that the HTTP Authentication protocol
allow this, so as it's not about a framework, but about a server which
intends to implement the protocol, we must not care about the 2 years
old kid :D

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



2010/8/19 Christian Grothoff <address@hidden>:
> Even that is not a problem with the proposed API since NULL != "".
>
> Best,
>
> Christian
>
> On Thursday, August 19, 2010 09:28:51 pm Carlos Henrique Júnior wrote:
>> 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-----
>