You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Justin Erenkrantz <ju...@erenkrantz.com> on 2005/12/02 06:53:07 UTC
Re: svn commit: r351547 - in /httpd/httpd/branches/authz-dev: include/http_core.h modules/aaa/mod_authz_host.c server/core.c server/request.c
On Fri, Dec 02, 2005 at 01:19:11AM -0000, bnicholes@apache.org wrote:
> Author: bnicholes
> Date: Thu Dec 1 17:19:07 2005
> New Revision: 351547
>
> URL: http://svn.apache.org/viewcvs?rev=351547&view=rev
> Log:
> Reimplement ap_some_auth_required as an optional function since the data has moved to mod_authz_host yet it is still needed by the request handler
>
> Modified:
> httpd/httpd/branches/authz-dev/include/http_core.h
> httpd/httpd/branches/authz-dev/modules/aaa/mod_authz_host.c
> httpd/httpd/branches/authz-dev/server/core.c
> httpd/httpd/branches/authz-dev/server/request.c
In general, woo-hoo. =)
> Modified: httpd/httpd/branches/authz-dev/include/http_core.h
> URL: http://svn.apache.org/viewcvs/httpd/httpd/branches/authz-dev/include/http_core.h?rev=351547&r1=351546&r2=351547&view=diff
> ==============================================================================
> --- httpd/httpd/branches/authz-dev/include/http_core.h (original)
> +++ httpd/httpd/branches/authz-dev/include/http_core.h Thu Dec 1 17:19:07 2005
> @@ -687,6 +687,7 @@
>
> APR_DECLARE_OPTIONAL_FN(const apr_array_header_t *, authz_host_ap_requires,
> (request_rec *r));
> +APR_DECLARE_OPTIONAL_FN(int, authz_some_auth_required, (request_rec *r));
I'm not quite clear on why we need to have an optional function here. (I'm
probably missing something obvious here.)
> /*
> APR_DECLARE_OPTIONAL_FN(const char *, authz_host_ap_auth_type, (request_rec *r));
> APR_DECLARE_OPTIONAL_FN(const char *, authz_host_ap_auth_name, (request_rec *r));
>
> Modified: httpd/httpd/branches/authz-dev/modules/aaa/mod_authz_host.c
> URL: http://svn.apache.org/viewcvs/httpd/httpd/branches/authz-dev/modules/aaa/mod_authz_host.c?rev=351547&r1=351546&r2=351547&view=diff
> ==============================================================================
> --- httpd/httpd/branches/authz-dev/modules/aaa/mod_authz_host.c (original)
> +++ httpd/httpd/branches/authz-dev/modules/aaa/mod_authz_host.c Thu Dec 1 17:19:07 2005
I know you probably don't want to hear this, but this module has nothing to
do with hosts any more. Want to reconsider a mod_authz_require or
something like that? ;-)
But, there's no way we should have the authz provider lookup code stuck
inside of mod_authz_host.
> @@ -520,6 +520,46 @@
> return conf->ap_requires;
> }
>
> +static int authz_some_auth_required(request_rec *r)
> +{
> + authz_host_dir_conf *conf = ap_get_module_config(r->per_dir_config,
> + &authz_host_module);
> + authz_provider_list *current_provider;
> + int req_authz = 1;
Shouldn't this be defaulting to 0? If there aren't any providers active or
interested, why return 1?
> + current_provider = conf->providers;
> + do {
> + const authz_provider *provider;
> +
> + /* For now, if a provider isn't set, we'll be nice and use the file
> + * provider.
> + */
> + if (!current_provider) {
> + provider = ap_lookup_provider(AUTHZ_PROVIDER_GROUP,
> + AUTHZ_DEFAULT_PROVIDER, "0");
> +
> + if (!provider || !provider->check_authorization) {
> + ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r,
> + "No Authz providers configured. Assmuming no authorization required.");
> + req_authz = 0;
> + break;
> + }
Considering we already do a check at config/registration time, do we really
need to repeat this check at request-time?
> + }
> + else {
> + provider = current_provider->provider;
> + }
> +
> + if (current_provider->method_mask & (AP_METHOD_BIT << r->method_number)) {
> + req_authz = 1;
> + break;
> + }
> +
> + current_provider = current_provider->next;
> + } while (current_provider);
The current version of the branch has a bunch of dead-code on it for this
loop. We should strip it to the bare minimum:
...
req_authz = 0;
provider = conf->providers;
while (provider) {
if (provider->method_mask & (AP_METHOD_BIT << r->method_number)) {
req_authz = 1;
break;
}
provider = provider->next;
}
return req_authz;
...
Yet, something unsettles me about having to do this check at all. However,
my thoughts haven't yet coalesced and I'm falling asleep at the keyboard.
I'll try to collect my thoughts on this and follow up later. -- justin
Re: Comments on Authz_Provider implementation (was: Re: svn commit: r351547 - in /httpd/h)
Posted by Andreas Lindström <an...@gmail.com>.
> Require directives in the form of:
>
> require user joe bob jane
> require ldap-user jmanager
> require ldap-group bigboys
> require valid-user
I might have misunderstood something here, but this sounds like you
are proposing to move all directives into the specific files for the
authproviders. (This sounds like a good idea btw, as it most likely
will make the auth system easier to follow since all provider specific
routines are in the providers own file.)
If this is indeed what is intended, then the thing that should be
needed in mod_auth_basic and mod_auth_digest is some kind of wrapper
function that determines which provider that is active and then calls
the functions that are provider specific. Or possibly a function that
retrieves the result from the provider and then applies it in an
authtype specific way. The last one is probably the way to go (if
possible) as it will mean less authtype specific code in the provider
files.
However, if you want to run several auth providers you will probably
need some kind of Satisfy directive somewhere. An idea i previously
brought up was to place the directive in mod_authn_alias (that
probably should change name then, perhaps mod_authn_instance or
something) and use a syntax similar to "Satisfy ((instance1 &
instance2) | instance3) & !passwd " as this will add a great deal of
flexibility to the auth system.
If you leave mod_authn_alias to work as it does atm (as an optional
part) you could still declare the access checks directly in the
location that needs it, and you wouldnt need to use Satisfy for that
either if the design is made carefully to allow interswapping to occur
between the Satisfy directive in the alias module and the Require
directive from the provider files (or some check in basic, depending
on how you design it).
If the auth providers all handle their own verification of users based
on the require directives all this satisfy directive would need to do
is get the results from the auth providers own checks and run the
operations on it (if any). This would effectively make it possible to
use several different authproviders and declare them as instances
(pretty much the same syntax as mod_authn_alias uses now) and then use
the satisfy directive to just calculate the access rights.
Possibly these instances could be made specific to an authtype as
well, which would allow users of httpd to mix different auth types and
then only use the satisfy directive to check if the user has access or
not.
This would also make the config file less complicated to read as all
instances could be declared in one place and then referenced from the
Virtual Hosts or wherever you want to use them. It would also make it
possible to add a specific file or other provider with users you DONT
want to be able to log in, for example the passwd file where all
system wide users and others are entered, or possibly just a new
provider where you enters all the users that has been banned or
whatever.
Something like:
AuthBasicProvider ldap1 file1 sql1 passwd
AuthDigestProvider prov2
Satisfy ((ldap1 | file1 | sql1) | prov2) & !passwd
or:
AuthProvider ldap1 file1 sql1 passwd prov2
Satisfy ((ldap1 | file1 | sql1) | prov2) & !passwd
would be all thats needed to exist in the folder/file/location that
the access rules apply for. (Depending on if the auth type is defined
in the instances or if the modules for auth_basic/auth_digest will
define which instances are of which type.)
> How could we fit IP restrictions into this?
>
> require ip !192.168.0.0/24 10.0.1.5
>
> Possibly have deny/allow from semantics 'silently' convert into require
> directives? Might be able to salvage backwards compat this way.
As for the ip access, it could be made into a pure bread auth module
if there was the will for it to happen as they would all handle their
own auth checks (or possibly a common auth function in some common lib
for all providers if theres alot of them that uses the same rules for
checking access. (this would make for less duplicate code, but
possibly make it less straightforward as well)
Another idea for ip access instances might be an idea to use Allow and
Deny, but in a different way:
Allow 10.0.1.5
Deny 192.168.0.0/24
Deny 100.0.0.0 255.255.255.255
or something like the above. This would allow one instance of the ip
access to include as many or as few of those directives as needed.
(And possibly also make it easier for users to understand the usage of
the ip access checker.)
The instances of providers would be handy here too as all provider
specific things would be gathered at one place, easier to get an
overlook over the auth rules and much less scrolling of potentially
(very!) long Virtual Host declarations (especially if you have some
rewrite rules in it). As for the ip access it would most likely need
the possibility of using several requires (or, see allow, deny above)
in one instance (shouldnt be a problem if the provider themselves
handle the authorization) or you could just use one require per
instance and then just use Satisfy to evaluate them as you want to.
"Satisfy (ip1 | ip2) & !ip3"
The silent conversion of allow and deny sounds like a good idea, at
least during a conversion period. Having both syntaxes active and
allowed at the same time shouldnt be much of a problem as long as an
example of the (soon to be) deprecated way and the new way are both
included in an example configuration file and the documentation.
Oh, as i havent had time to look into the auth system yet i dont have
that much of a clue as to how the current design is made, so much of
this might be insane ideas or whatever. If some of this seems
plausible to work and youd like to know more id be happy to draw up
some sort of a basic flow chart detailing as much of this idea as
possible.
All of this is so far only a basic idea of how it could be made to
work, for it to actually work some more time and thinking has to take
place. But as i havent heard any comments im not even sure if this is
plausible or not.
Long mail again... phew.
/ Andreas
Re: Comments on Authz_Provider implementation (was: Re: svn commit:
r351547 - in /httpd/h)
Posted by Justin Erenkrantz <ju...@erenkrantz.com>.
--On December 2, 2005 9:39:46 AM -0700 Brad Nicholes <BN...@novell.com>
wrote:
> As I mentioned in my last commit, it still needs some clean up.
> Please, Please feel free to jump in and clean it up wherever you see the
> need. I don't have all of the answers to why things were done the way
Okay, there are a bunch of style nits that I'll go clean up if I get a free
moment.
> they were before and if we still need to do it that way now or is there
> a better way. In this branch I have been trying to maintain backward
> API compatibility as much as it makes sense. But there are questions I
> have about the need of some APIs such as ap_requires() and
> ap_some_auth_required(), etc. These are points that I need feedback on
> and if we can eliminate them, like as was discussed with AuthType, then
> lets do it.
I have these exact same questions - so we're struggling with the same
things. Good.
Below are the unedited rambling notes I wrote last night as I was falling
asleep. Feel free to comment. =) -- justin
=================
Def'n:
authentication: who is this user?
authorization: is this user authorized?
access: run before authn/authz; but really can be categorized in
authorization
------
Goal:
Want to run authorization hook even on anonymous read-only requests *and*
also when r->user is set.
------
Current behavior:
satisfy all / not spec
access
if auth required
check_user_id
auth_checker
satisfy any
access
if fails:
if auth not required: bail
if check_user_id fails: bail
if auth_checker fails: bail
------
Some auth required is to prevent 'expensive' lookups from occurring.
Q: Is this really needed with a provider system?
------
Ideal behavior?
Restrict check_user_id hook for Digest/Basic/Auth mechanisms
As needed, auth mech. runs through conf authn providers
All configured authn providers are executed in specific order
Will not execute authn providers unless credentials presented
run through all authz providers for the Location, respecting Limit
All configured authz providers are executed in specific order
Remove hooks?
Move access checker to authz providers
Only mod_authz_host was an access checker in our tree anyway
Can/should we purposely break backwards-compat for authz modules in
next revision?
------
Require directives in the form of:
require user joe bob jane
require ldap-user jmanager
require ldap-group bigboys
require valid-user
------
mod_authz_svn has both access and authz hooks
authz hook is RUN_FIRST - ugh
*NO* require directive
require svn-group ?
------
How could we fit IP restrictions into this?
require ip !192.168.0.0/24 10.0.1.5
Possibly have deny/allow from semantics 'silently' convert into require
directives? Might be able to salvage backwards compat this way.
-------
Comments on Authz_Provider implementation (was: Re: svn
commit: r351547 - in /httpd/h)
Posted by Brad Nicholes <BN...@novell.com>.
As I mentioned in my last commit, it still needs some clean up.
Please, Please feel free to jump in and clean it up wherever you see the
need. I don't have all of the answers to why things were done the way
they were before and if we still need to do it that way now or is there
a better way. In this branch I have been trying to maintain backward
API compatibility as much as it makes sense. But there are questions I
have about the need of some APIs such as ap_requires() and
ap_some_auth_required(), etc. These are points that I need feedback on
and if we can eliminate them, like as was discussed with AuthType, then
lets do it.
>>> On 12/1/2005 at 10:53:07 pm, in message
<20...@scotch.ics.uci.edu>, justin@erenkrantz.com
wrote:
>
=============================================================================
> =
>> --- httpd/httpd/branches/authz-dev/include/http_core.h (original)
>> +++ httpd/httpd/branches/authz-dev/include/http_core.h Thu Dec 1
17:19:07
> 2005
>> @@ -687,6 +687,7 @@
>>
>> APR_DECLARE_OPTIONAL_FN(const apr_array_header_t *,
authz_host_ap_requires,
>> (request_rec *r));
>> +APR_DECLARE_OPTIONAL_FN(int, authz_some_auth_required, (request_rec
*r));
>
> I'm not quite clear on why we need to have an optional function here.
(I'm
> probably missing something obvious here.)
The optional functions are here because the data moved but core still
needs it in order to process the request. If we can eliminate the need
for core to require this information, then lets do it and get rid of the
optional functions. The authz_host_ap_requires() optional function
(used in the ap_requires() API) can probably be removed in the clean up
since the "Require" line is no longer handled like it was before. In
other words, there isn't a need to keep or retrieve an array of
"Require" lines that must be parsed at request time. The only purpose
for the ap_require() function was to get the array of "Require" lines
for a particular dir_config.
>
=============================================================================
> =
>> --- httpd/httpd/branches/authz-dev/modules/aaa/mod_authz_host.c
(original)
>> +++ httpd/httpd/branches/authz-dev/modules/aaa/mod_authz_host.c Thu
Dec 1
> 17:19:07 2005
>
> I know you probably don't want to hear this, but this module has
nothing to
> do with hosts any more. Want to reconsider a mod_authz_require or
> something like that? ;-)
>
> But, there's no way we should have the authz provider lookup code
stuck
> inside of mod_authz_host.
>
I'm not opposed to creating a new mod_authz_require module if it is
necessary. My only concern is auth_module overload. It just seems
wrong to have to load 5-6 different authx_modules just to get simple
authentication working. I chose to move the "Require" directive into
mod_authz_host simply thinking that "host" would refer to non-authz
specific module functionality (ie. a catch-all for global authz
functionality). But I can be easily convinced to put "Require" along
with "AuthName" somewhere else if needed. I need opinions and feedback
on this issue please.
>> @@ -520,6 +520,46 @@
>> return conf->ap_requires;
>> }
>>
>> +static int authz_some_auth_required(request_rec *r)
>> +{
>> + authz_host_dir_conf *conf =
ap_get_module_config(r->per_dir_config,
>> + &authz_host_module);
>> + authz_provider_list *current_provider;
>> + int req_authz = 1;
>
> Shouldn't this be defaulting to 0? If there aren't any providers
active or
> interested, why return 1?
>
True, it should default to 0.
>> + current_provider = conf->providers;
>> + do {
>> + const authz_provider *provider;
>> +
>> + /* For now, if a provider isn't set, we'll be nice and use
the file
>> + * provider.
>> + */
>> + if (!current_provider) {
>> + provider = ap_lookup_provider(AUTHZ_PROVIDER_GROUP,
>> + AUTHZ_DEFAULT_PROVIDER,
"0");
>> +
>> + if (!provider || !provider->check_authorization) {
>> + ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r,
>> + "No Authz providers configured.
Assmuming no
> authorization required.");
>> + req_authz = 0;
>> + break;
>> + }
>
> Considering we already do a check at config/registration time, do we
really
> need to repeat this check at request-time?
This is another area that needs some clean up. This is kind of a cut
and paste left-over from the authnprovider stuff. If a "Require" line
does not exist, in other words no authz provider was requested, should
we attempt to default to something like 'valid-user' in the same way
that the authn stuff defaults to 'file'? As it stands now, attempting
to default would never happen because of the call to
ap_some_auth_required() during the request processing that skips authz
checking if there isn't a "Require" line present.
>
>> + }
>> + else {
>> + provider = current_provider->provider;
>> + }
>> +
>> + if (current_provider->method_mask & (AP_METHOD_BIT <<
r->method_number)) {
>> + req_authz = 1;
>> + break;
>> + }
>> +
>> + current_provider = current_provider->next;
>> + } while (current_provider);
>
> The current version of the branch has a bunch of dead-code on it for
this
> loop. We should strip it to the bare minimum:
Not sure what dead code you are referring to, but by all means, strip
it out if it isn't needed.
>
> ...
> req_authz = 0;
> provider = conf->providers;
> while (provider) {
> if (provider->method_mask & (AP_METHOD_BIT << r->method_number)) {
> req_authz = 1;
> break;
> }
> provider = provider->next;
> }
>
> return req_authz;
> ...
>
> Yet, something unsettles me about having to do this check at all.
However,
> my thoughts haven't yet coalesced and I'm falling asleep at the
keyboard.
> I'll try to collect my thoughts on this and follow up later. --
justin
The 'method_mask' check is there so that if the "Require" line is
enclosed in a <Limit> block, we only apply authorization for the
appropriate methods. We may be able to consolidate this to the
auth_checker hook function before the providers are called and then
eliminate this check in the authz providers.
Like I mentioned before, Please, Please jump in and help out here. I
haven't got all of the answers, I just want to see this functionality
implemented and hopefully improve the authn/authz handling.
Brad