Re: [libmicrohttpd] Re: HTTP Digest Auth done

[Christian Grothoff]

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