[libmicrohttpd] Re: HTTP Digest Auth done

[Christian Grothoff]

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