Re: Regarding authentication scheme handling

Pallavi Agrawal wrote:
> Hi,
>   
Aloha!
> I am working on libwww.
>   
Most excellent . . .
> I have one query related to libwww handling of NTLM responses for
> authentication.
>
> Looking at the code in HTInit.c, it seems that only "basic" and "digest"
> authentication schemes are supported.
>
>  
>
> PUBLIC void HTAAInit (void)
>
> {
>
>  
>
>     HTAA_newModule ("basic", HTBasic_generate, HTBasic_parse, NULL,
>
>                              HTBasic_delete);
>
> #ifdef HT_MD5
>
>     HTAA_newModule ("digest", HTDigest_generate, HTDigest_parse, 
>
>                              HTDigest_updateInfo,  HTDigest_delete);
>
> #endif /* HT_MD5 */
>
> }
>   
Yes, it would be nice to have a few more authentication modules to
handle bits not handled nicely from RFC 2617, or other schemes found in
the wild.  Is there a nice specification for NTLM ?

It seems that using this an an authentication handler would force
connection keep alive and perhaps some prior state tracking to determine
when an authentication for some METHOD + URI has already occurred or
that a client initiated reauthentication is expected.

[: Note :  There were some reported examples of bad things possible when
using NTLM with particular configurations of specific proxy servers,
though that is more an argument against discarding Via headers, than the
use of weak authentication mechanisms.  After all, Basic authentication
is supported :]

> HTAuthInfoFilter function checks for the available modules(which are basic
> and digest in this case). If the current scheme doesn't match with any of
> them, it will return HT_ERROR. In HTNetcall_executeAfter, this return type
> is not handled going further up.
>
>  
>
> PUBLIC int HTNetCall_executeAfter (HTList * list, HTRequest * request,
>
>                                                    int status)
>
> {
>
>     int ret = HT_OK;
>
>     if (status != HT_IGNORE) {
>
>             HTParentAnchor * anchor = HTRequest_anchor(request);
>
>             char * url = HTAnchor_physical(anchor);
>
>             char * addr = url ? url : HTAnchor_address((HTAnchor *) anchor);
>
>             HTResponse * response = HTRequest_response(request);
>
>             if (list && request && addr) {
>
>                 AfterFilter * pres;
>
>                 while ((pres = (AfterFilter *) HTList_nextObject(list))) {
>
>                         if ((pres->status == status || pres->status ==
> HT_ALL) &&
>
>                             (!pres->tmplate ||
>
>                              (pres->tmplate && HTStrMatch(pres->tmplate,
> addr)))) {
>
>                             HTTRACE(CORE_TRACE, "Net After... calling %p
> (request %p, response %p, status %d, context %p)\n" _ 
>
>                                                 pres->after _ request _
> response _ 
>
>                                                 status _ pres->param);
>
>                             ret = (*pres->after)(request, response,
> pres->param, status);
>
>                             if (ret != HT_OK) break;
>
>  
>
>                             /*
>
>                             **  Update the address to match against if the
> filter changed
>
>                             **  the physical address.
>
>                             */
>
>                             if ((url = HTAnchor_physical(anchor))) addr =
> url;
>
>                         }
>
>                 }
>
>             }
>
>           
>
>  
>
>             if (!url) {
>
>                         HT_FREE(addr); 
>
>             }
>
>     }
>
>     return ret;
>
> }
>
> There are many places in code where the return type from this function is
> not handled.
>
> For example in function HTNet_newClient, following code doesn't handle the
> return value:
>
> if (HTEvent_isCallbacksRegistered() && !HTRequest_preemptive(request))
>
>                 createAfterFilterEvent(request, status);
>
>             else
>
>                 HTNet_executeAfterAll(request, status);
>
>             return YES;
>
> This is resulting into browser getting hung and the select call in
> HTEvtLst_loop doesn't return. 
>   
Yikes, perhaps a patch is indeed required.  Have you already coded an
appropriate set of error handling checks ?

Just peering into the code based on your observations we can see,

  $ find . | egrep "\.(c|h)$" | xargs grep HTNet_executeAfterAll
  ./Library/src/HTNet.c:PUBLIC int HTNet_executeAfterAll (HTRequest *
request, int status)
  ./Library/src/HTNet.c:    return HTNet_executeAfterAll(request, status);
  ./Library/src/HTNet.c:        HTNet_executeAfterAll(request, status);
  ./Library/src/HTNet.c:        HTNet_executeAfterAll(request, status);
  ./Library/src/HTNet.c:    if (status != HT_IGNORE)
HTNet_executeAfterAll(request, status);
  ./Library/src/HTNet.h:extern int HTNet_executeAfterAll (HTRequest *
request, int status);

The last three occurrences in HTNet.c require inspection.  The first of
these is in the context of the function HTNet_executeAfterAll, which is
in the scope of handling cases where the HTNet_executeBeforeAll function
has not returned HT_OK.  The second of these in the context of
HTNet_newClient is a near identical usage.  The last of the occurrences,
in the context of HTNet_delete, seems to suppose that the after filters
would gain the relevant information from the Net object itself, prior to
it's actual deletion. 

How are we to proceed if the after filters are not OK ?

Looking more closely at the definition of HTNet_executeAfterAll, it is
apparent that the real work is done by HTNetCall_executeAfter which
breaks it's iteration the first time an after filter returns not HT_OK. 
This really seems to put us in a difficult spot trying to identify
_which_ after filter would have been the one to signal failure.

The one additional bit of code to examine is along the path of which
functions might actually get registered along the way.  Namely, we
search for invocations of HTNetCall_addAfter and therefore at the
invocations of HTRequest_addAfter, e.g.,

  $ find . | egrep "\.c$" | xargs grep HTRequest_addAfter

My inclination is to first restrict the reading of code in the Library,
thus only reading the additional files HTAccess.c, HTCache.c and
HTMIME.c . . .

The intent to add an NTLM authentication might be done as a seperate
add-on, since registering the new after filters after core library
initialization will naturally cause them to be executed first at
runtime.  An implementation of an NTLM enabled equivalent of
HTAuthFilter could also track prior session state, somewhat like the
HTMemoryCacheFilter.

more,
l8r,
v


-- 
"The future is here. It's just not evenly distributed yet."
 -- William Gibson, quoted by Whitfield Diffie

Received on Tuesday, 7 April 2009 02:17:59 UTC