You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by David Shane Holden <dp...@apache.org> on 2008/07/11 08:26:27 UTC
Re: svn commit: r667651 - /httpd/httpd/trunk/modules/aaa/mod_authz_core.c
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?rev=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.
> @@ -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?
>
> 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
>
>
>
>
>
Re: svn commit: r667651
- /httpd/httpd/trunk/modules/aaa/mod_authz_core.c
Posted by Brad Nicholes <BN...@novell.com>.
>>> On 7/11/2008 at 5:30 PM, in message
<5A...@gbiv.com>, "Roy T. Fielding"
<fi...@gbiv.com> wrote:
> On Jul 11, 2008, at 2:14 PM, Brad Nicholes wrote:
>
>>>>> On 7/11/2008 at 12:01 PM, in message
>>>>> <48...@apache.org>, David Shane
>> Holden <dp...@apache.org> wrote:
>>> Thanks for the link and description Brad. It makes sense now.
>>> Explains
>>> why the default config was giving me a 403. The 'Require all denied'
>>> was being inherited from the root directory config. Would it be
>>> appropriate to add something like the attached patched to
>>> httpd.conf.in?
>>
>> In this case, probably.
>
> The default needs to be off. We can't disable sites on an upgrade
> within
> the 2.x series.
>
> ....Roy
So this was really the question that was being discussed especially in the last few messages of the thread http://www.mail-archive.com/dev%40httpd.apache.org/msg40286.html. Is it better to switch the default to ON knowing that 2.4 might disable some sites based on stricter auth rules, or leave the default at OFF knowing that there might be some holes left open? Maybe the justification is that the holes where always there anyway and being plugged by extra auth configuration prior to 2.4, so 2.4 really doesn't need to enforce stricter auth rules. I intentionally wrote the patch so that both the defaults for the AuthzMergeRules directive and the initial merge rule, can be easily switched. I would just ask that those concerned read through the message thread and determine what the defaults should be. I can see pros and cons of each but I can go with whatever makes sense to the user.
Brad
Brad
Re: svn commit: r667651 - /httpd/httpd/trunk/modules/aaa/mod_authz_core.c
Posted by "Roy T. Fielding" <fi...@gbiv.com>.
On Jul 11, 2008, at 2:14 PM, Brad Nicholes wrote:
>>>> On 7/11/2008 at 12:01 PM, in message
>>>> <48...@apache.org>, David Shane
> Holden <dp...@apache.org> wrote:
>> Thanks for the link and description Brad. It makes sense now.
>> Explains
>> why the default config was giving me a 403. The 'Require all denied'
>> was being inherited from the root directory config. Would it be
>> appropriate to add something like the attached patched to
>> httpd.conf.in?
>
> In this case, probably.
The default needs to be off. We can't disable sites on an upgrade
within
the 2.x series.
....Roy
Re: svn commit: r667651
- /httpd/httpd/trunk/modules/aaa/mod_authz_core.c
Posted by Brad Nicholes <BN...@novell.com>.
>>> On 7/11/2008 at 12:01 PM, in message <48...@apache.org>, David Shane
Holden <dp...@apache.org> wrote:
> Thanks for the link and description Brad. It makes sense now. Explains
> why the default config was giving me a 403. The 'Require all denied'
> was being inherited from the root directory config. Would it be
> appropriate to add something like the attached patched to httpd.conf.in?
In this case, probably.
Brad
Re: svn commit: r667651 - /httpd/httpd/trunk/modules/aaa/mod_authz_core.c
Posted by David Shane Holden <dp...@apache.org>.
Thanks for the link and description Brad. It makes sense now. Explains
why the default config was giving me a 403. The 'Require all denied'
was being inherited from the root directory config. Would it be
appropriate to add something like the attached patched to httpd.conf.in?
Re: svn commit: r667651 -
/httpd/httpd/trunk/modules/aaa/mod_authz_core.c
Posted by Brad Nicholes <BN...@novell.com>.
See inline comments below.
Brad
>>> On 7/11/2008 at 12:26 AM, in message <48...@apache.org>, David Shane
Holden <dp...@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
>>
>>
>>
>>
>>