httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Brad Nicholes" <BNICHO...@novell.com>
Subject Re: svn commit: r667651 - /httpd/httpd/trunk/modules/aaa/mod_authz_core.c
Date Fri, 11 Jul 2008 14:31:40 GMT
See inline comments below.

Brad

>>> On 7/11/2008 at 12:26 AM, in message <4876FD13.60401@apache.org>, David
Shane
Holden <dpejesh@apache.org> wrote:
> I tried to build Apache from trunk tonight and noticed that this patch 
> broke something.  I'm getting a 403 error when trying to browse to a 
> clean install.  I'm by no means an expert here, but I noticed a few 
> things which are noted below...
> 
> bnicholes@apache.org wrote:
>> Author: bnicholes
>> Date: Fri Jun 13 13:59:10 2008
>> New Revision: 667651
>>
>> URL: http://svn.apache.org/viewvc?rev=667651&view=rev 
>> Log:
>> Switch the default base authz logic operation to 'AND' rather than 'OR'.  
> This should allow directory authz rules merging to be more restrictive in 
> sub-directories
>>
>> Modified:
>>     httpd/httpd/trunk/modules/aaa/mod_authz_core.c
>>
>> Modified: httpd/httpd/trunk/modules/aaa/mod_authz_core.c
>> URL: 
> http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/aaa/mod_authz_core.c?r 
> ev=667651&r1=667650&r2=667651&view=diff
>> 
> =============================================================================
> =
>> --- httpd/httpd/trunk/modules/aaa/mod_authz_core.c (original)
>> +++ httpd/httpd/trunk/modules/aaa/mod_authz_core.c Fri Jun 13 13:59:10 2008
>> @@ -111,13 +111,16 @@
>>  static const char *merge_authz_provider(authz_core_dir_conf *conf, 
> authz_provider_list *newp);
>>  static void walk_merge_provider_list(apr_pool_t *a, authz_core_dir_conf 
> *conf, authz_provider_list *providers);
>>  
>> +#define BASE_REQ_STATE AUTHZ_REQSTATE_ALL
>> +#define BASE_REQ_LEVEL 0
>> +
>>  static void *create_authz_core_dir_config(apr_pool_t *p, char *dummy)
>>  {
>>      authz_core_dir_conf *conf =
>>              (authz_core_dir_conf *)apr_pcalloc(p, 
> sizeof(authz_core_dir_conf));
>>  
>> -    conf->req_state = AUTHZ_REQSTATE_ONE;
>> -    conf->req_state_level = 0;
>> +    conf->req_state = BASE_REQ_STATE;
>> +    conf->req_state_level = BASE_REQ_LEVEL;
>>      conf->merge_rules = 1;
>>      return (void *)conf;
>>  }
>>   
> Not sure if this was intentional... but the default went from 
> authz_reqstate_one to authz_reqstate_all.  If I change base_req_state to 
> authz_reqstate_one the 403 disappears, but since I don't know much about 
> how this is suppose to work it might not be the correct fix.

Yes, the switch from AUTHZ_REQSTATE_ONE to AUTHZ_REQSTATE_ALL was intentional.  It was also
known that doing this might cause some problems for existing configurations.  But the switch
was made to close up some security issues.  Take a look at this thread for a complete description
of how things work and why. http://www.mail-archive.com/dev%40httpd.apache.org/msg40286.html

>> @@ -180,11 +183,21 @@
>>  
>>      /* Walk all of the elements recursively to allow each existing
>>          element to be copied and merged into the final configuration.*/
>> -    if (providers->one_next) {
>> -        walk_merge_provider_list (a, conf, providers->one_next);
>> +    if (BASE_REQ_STATE == AUTHZ_REQSTATE_ONE) {
>> +        if (providers->one_next) {
>> +            walk_merge_provider_list (a, conf, providers->one_next);
>> +        }
>> +        if (providers->all_next) {
>> +            walk_merge_provider_list (a, conf, providers->all_next);
>> +        }
>>      }
>> -    if (providers->all_next) {
>> -        walk_merge_provider_list (a, conf, providers->all_next);
>> +    else {
>> +        if (providers->all_next) {
>> +            walk_merge_provider_list (a, conf, providers->all_next);
>> +        }
>> +        if (providers->one_next) {
>> +            walk_merge_provider_list (a, conf, providers->one_next);
>> +        }
>>      }
>>   
> base_req_state == authz_reqstate_one will always fail.  was this 
> comparison suppose to be conf->req_state == authz_reqstate_one?

No, the #define BASE_REQ_STATE was added so that the code could be easily switched back to
a default of AUTHZ_REQSTATE_ONE if testing proved that a default of AUTHZ_REQSTATE_ALL was
incorrect.  With the default set to AUTHZ_REQSTATE_ALL you are correct, this statement will
always fail.  But if the default were switched back, then it makes sense.  Once everything
has been tested and the correct default state has been determined, then this statement can
be cleaned up if necessary.

>>  
>>      return;
>> @@ -200,18 +213,30 @@
>>          authz_provider_list *last = conf->providers;
>>          int level = conf->req_state_level;
>>  
>> -        /* if the level is 0 then take care of the implicit 'or'
>> +        /* if the level is the base level then take care of the implicit 
>>           * operation at this level. 
>>           */
>> -        if (level == 0) {
>> -            /* Just run through the Require_one list and add the
>> -             * node 
>> -             */
>> -            while (last->one_next) {
>> -                last = last->one_next;
>> +        if (level == BASE_REQ_LEVEL) {
>> +            if (conf->req_state == AUTHZ_REQSTATE_ONE) {
>> +                /* Just run through the Require_one list and add the
>> +                 * node 
>> +                 */
>> +                while (last->one_next) {
>> +                    last = last->one_next;
>> +                }
>> +                last->one_next = newp;
>> +            }
>> +            else {
>> +                /* Just run through the Require_all list and add the
>> +                 * node 
>> +                 */
>> +                while (last->all_next) {
>> +                    last = last->all_next;
>> +                }
>> +                last->all_next = newp;
>>              }
>> -            last->one_next = newp;
>>          } 
>> +
>>          /* if the last nodes level is greater than the new nodes 
>>           *  level, then we need to insert the new node at this
>>           *  point.  The req_state of the new node determine
>>
>>
>>
>>
>>   



Mime
View raw message