httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Chris Darroch <chr...@pearsoncmg.com>
Subject Re: walk caching to avoid extra authnz
Date Wed, 06 Dec 2006 23:42:03 GMT
Hi --

   Thanks for taking an initial look at these patches; I reviewed
them a bit more as well and did some testing this morning which
resulted in a pair of small changes.  One of those changes is important;
it catches the condition where the current walk finds additional
matches beyond those which matched for the previously cached walk.
That's a condition which doesn't matter with the current code,
where you always get a private r->per_dir_config unless the URI or
filename is an exact match with the one in the parent request.
But with these patches, it matters because it's another case where
you can't reuse the parent's r->per_dir_config.

   Based on Justin's and Nick's comments, below, I confess that I
feel rather stuck between a rock and a hard place.  On the one hand,
enabling mod_dav and mod_authn_dbd amounts to a kind of denial-of-
service problem: DAV clients can issue legitimately issue requests
that cause large numbers of DB queries, one for each item in a
directory.  (I suppose mod_autoindex might have similar problems,
but without the extra recursive kick in the pants.)

   On the other hand, as Justin points out, some existing modules
like mod_authz_svn may rely on httpd's current behaviour, in which
sub-requests with distinct URIs from their parents always go through
the authnz steps.  Some thoughts on that score at the bottom of
this message.


Nick Kew wrote:

> There's a comment in both 2.2.x and trunk, just at the start of your
> patch, saying:
> 
>     /* Skip authn/authz if the parent or prior request passed the
> authn/authz,
>      * and that configuration didn't change (this requires optimized
> _walk()
>      * functions in map_to_storage that use the same merge results given
>      * identical input.)  If the config changes, we must re-auth.
>      */
> 
> which looks like exactly what your patch is doing.  WTF?

   Yeah, that's surprising, it's it?  :-)  But that comment (not part
of the patch, but in the code for a long time) seems not to mean what
you might think it means.  The applicable config directives can be
identical but the ap_*_walk() functions still merge a new, private
r->per_dir_config for any sub-request or internal redirect where the
URI or filename isn't precisely the same as the parent's.

   That's why things like mod_dav which trigger lots of sub-requests
effectively can't be used with any modules that do complicated authnz
(mod_authn_dbd, say, or maybe an LDAP thing).  In our case, we could
see thousands of redundant DB queries from a single DAV request;
it's basically a non-starter without some kind of fix.


> Second, there are substantial changes in directory_walk which I
> would expect to affect this.  Did you observe the problem behaviour
> in both 2.2.x and trunk?

   In our case, it's actually all about ap_location_walk(), so yes,
both 2.2.x and trunk have the same behaviour.  It's essentially
independent of what kinds of specific transformations and checks
the walks do.  It has more to do with what happens when a path like
/foo/bar/ matches exactly the same set of configuration sections
as a parent request like /foo/.  From the code comments, you might
think it would get the parent's r->per_dir_config, but that's not
what happens.


> Alternative proposal for this scenario that doesn't involve a possible
> risk of breaking something.
> 
> mod_auth_inherit
> 
> In anything that isn't a subrequest, it'll return DECLINED.
> 
> In a subrequest, mod_auth_inherit will set r->user to r->main->user,
> without reference to any password lookup.  If r->main->user is
> unset it'll return DECLINED.
> 
> It'll also set an "inherited" token.
> 
> A corresponding authz hook will implement a "Require inherit"
> to enable subrequests with "inherited" set to be authorized,
> and will run ahead of "normal" authz hooks.

   Well, such a thing might work, but at first blush it seems to me
like it puts an undue burden on administrators, who'd have to read
the fine print to determine that a particular combination of authnz
modules with other modules might produce serious denial-of-service
consequences on their server, and that they need to install and
correctly configure additional modules to prevent this.


Justin Erenkrantz wrote:

> BTW, I expect that this would break Subversion and mod_authz_svn.  It
> is possible to have an authz setup such that I can read /foo, but not
> /foo/bar - the *only* way to confirm that someone has access is via
> the authz hooks (and why SVN has such a penalty for path authz).  Yet,
> if this breaks that, then that's a no-no.  So, have you tested that
> setup and confirmed that it still works?  -- justin

   Indeed it does break this; thanks for the heads-up.  I sort of feel
like such modules are relying on less than ideal behaviour on httpd's
part.  To me at least, it seems like common sense that a config section
like this implies one DB authn lookup per request:

<Location /foo/>
   Dav filesystem
   AuthDigestProvider dbd
   Require valid-user
   ...
</Location>

   The same is true of mod_authz_svn, I suppose; in fact, the Subversion
documentation has its own "fine print" section which warns people about
the performance consequences of, in essence, issuing a lot of sub-requests
when combined with authnz in a config section like the one below:

http://svnbook.red-bean.com/nightly/en/svn-book.html#svn.serverconfig.httpd.authz.pathauthzoff

<Location /foo/>
    Dav svn
    SVNPath /foo/
    Require valid-user
    AuthzSVNAccessFile /authz
    ...
</Location>

   Part of the trouble here is the fact that the AuthzSVNAccessFile
can specify particular authorizations for particular file paths,
effectively becoming an extension of the <Directory>, <Location>,
and <Files> sections of httpd's configuration.


   Now, I'm likely going to go ahead with my patch on our servers,
because I know we definitely don't have anything like that going on,
and definitely do want to avoid excessive DB overhead.

   One thing which occurs to me is that configuration sections
could be tagged as inheritable or not, depending on whether they
contain directives from modules which register authnz hooks/providers
and which do not, explicitly, indicate that they don't "extend"
the per-URI configurations (in the manner of AuthzSVNAccessFile).
Thus, existing modules mod mod_authz_svn would continue to get
authnz calls for every sub-request with a distinct URI.  However,
modules like mod_authz_user in the httpd distribution would set
this flag, so that most administrator's configurations would get
the benefit of the additional walk caching and lack of authnz
overhead for sub-requests.  Thoughts?

Chris.

-- 
GPG Key ID: 366A375B
GPG Key Fingerprint: 485E 5041 17E1 E2BB C263  E4DE C8E3 FA36 366A 375B


Mime
View raw message