Re: Comments on Authz_Provider implementation (was: Re: svn commit: r351547 - in /httpd/h)
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
Comments on Authz_Provider implementation (was: Re: svn commit: r351547 - in /httpd/h)
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
[EMAIL PROTECTED], [EMAIL PROTECTED]
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
Re: Comments on Authz_Provider implementation (was: Re: svn commit: r351547 - in /httpd/h)
--On December 2, 2005 9:39:46 AM -0700 Brad Nicholes [EMAIL PROTECTED] 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. ---
