Re: [libmicrohttpd] Re: HTTP Digest Auth done

[Amr Ali]

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



On 08/19/2010 09:28 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
> 
I'm not sure how to respond to this claim, its just that the concept of
authentication itself must include an identification for the party interested in
authenticating itself along with a secret shared between the two parties to
ensure authenticity. Now I understand that there were protocols (early 1980's
I'm not even sure of that) that authenticated based only on a single secret, of
course that's a flaw all in itself, but maybe it did exist.

Now are you asking for allowing to accept authentications with no usernames?
(its not possible in Digest Auth as it is a key component in generating the
response.). Security awareness is not at its best, people still use passwords
like "1234" on a scale bigger than you could imagine, so allowing what you are
asking for is like giving a gun to a 2 years old child and hope he/she won't
kill him/her self. It is already bad as it is, why do you think I should allow
degrading security even more? you have an application in mind that it would be a
problem if no-username is not allowed?! I very much doubt it.
> 
> 
> On Thu, Aug 19, 2010 at 8:46 AM, Amr Ali <address@hidden> wrote:
> 
> 
> 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)

iEYEARECAAYFAkxtiYwACgkQ2VxGY2Vcpoj8cACeMwNUaS0wvWqGmhVj/Cuc7xic
UuUAnRhmqRxbZPnpKvsxEW1yXX4QPsnE
=KM7A
-----END PGP SIGNATURE-----