[libmicrohttpd] Re: HTTP Digest Auth done

[Amr Ali]

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



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.
> 
> +/* 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
> 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,
> 
> +     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.
> 
> +/**
> + * 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?

> 
> // 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?
> 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 ;-)
> 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)

iEYEARECAAYFAkxtE6IACgkQ2VxGY2VcpojXuQCfcZEUapkWuJ1qdqhwgygf5dn5
RG0An0hWPKkLmJNI+aCA13jZeCgz8VKb
=bv3O
-----END PGP SIGNATURE-----