libmicrohttpd
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

[libmicrohttpd] Re: HTTP Digest Auth done


From: Christian Grothoff
Subject: [libmicrohttpd] Re: HTTP Digest Auth done
Date: Thu, 19 Aug 2010 10:08:25 +0200
User-agent: KMail/1.13.3 (Linux/2.6.32-trunk-vserver-amd64; KDE/4.4.4; x86_64; ; )

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


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

Also 'const char *bin'.

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


+
+       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).


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


+/**
+ * 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)


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


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

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



reply via email to

[Prev in Thread] Current Thread [Next in Thread]