You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Kamesh Jayachandran <ka...@collab.net> on 2008/03/07 16:02:14 UTC
[PATCH] Make mod_authz_svn to apply the authz checks against upper/lowercased
usernames
Hi All,
People using mod_auth_sspi(windows domain authentication apache module)
face the 'Authorization Failed' error while
accessing SVN with mixed case usernames which does not match in *exact
case* with their authz rules.
http://blog.michaelcheng.idv.hk/ explains it in detail.
mod_auth_sspi has a directive by name 'SSPIUsernameCase' with possible
values being 'Lower/Upper'.
I am not sure whether it really uses it to compare the
lowered/uppercased username.
I feel we should have similar directive in mod_authz_svn, to author the
*sane* authz rules in these mixed
case username scenarios.
Attached patch accomplishes that.
Want to know what community thinks about it.
With regards
Kamesh Jayachandran
Re: [PATCH] Make mod_authz_svn to apply the authz checks against upper/lowercased usernames
Posted by "Daniel L. Rall" <dl...@finemaltcoding.com>.
On Thu, 13 Mar 2008, Kamesh Jayachandran wrote:
>
> >I have some simplifications for that (attached, untested).
> >
> > while (*c) {
> >- *c = convert_func(*c);
> >+ *c = (to_uppercase ? apr_toupper(*c) : apr_tolower(*c));
> > ++c;
>
>
> I thought of this approach, but stayed away from it as 'to_upper check'
> happens for every character in a loop.
So? It's a single boolean check. The gain in simplicity far outweighs any
micro-optimization.
Re: [PATCH] Make mod_authz_svn to apply the authz checks against
upper/lowercased usernames
Posted by Kamesh Jayachandran <ka...@collab.net>.
> I have some simplifications for that (attached, untested).
>
> while (*c) {
> - *c = convert_func(*c);
> + *c = (to_uppercase ? apr_toupper(*c) : apr_tolower(*c));
> ++c;
I thought of this approach, but stayed away from it as 'to_upper check'
happens for every character in a loop.
Thanks
With regards
Kamesh Jayachandran
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Make mod_authz_svn to apply the authz checks against upper/lowercased usernames
Posted by "Daniel L. Rall" <dl...@finemaltcoding.com>.
On Wed, 12 Mar 2008, Kamesh Jayachandran wrote:
...
> >This sounds fairly reasonable to me. One big question I have is how
> >usernames
> >are then recorded in the history (e.g. uppercase, lowercase, or as entered
> >by the user?).
>
> Usernames are recorded as provided by the user. I did not want to change
> the request_rec->user.
Yeah, I'm a bit leary of that too. It would be nice to have some record of
what username the authentication occurred against, though. And I can't think
of anywhere better than the history to record that (since it provides an
accurrate audit trail). I guess I still favor changing it; it'd be nice to
hear from Justin or Sander or someone else more familiar with the ramifications
of doing something like that.
...
> >Ditto for the name of the directive, and even more so for the help (which
> >should mention that the case is applied to authz).
> >
> >
>
> Changed the directive to 'AuthzForceUsernameCase'.
+1
> >> (convert_to_uppercase_string,
> >> convert_to_lowercase_string,
> >> get_username_to_authorize): New functions.
> >>
> >
> >Do we really need both of those conversion functions? Seems like one
> >should be sufficient.
>
> Yes fixed that too.
I have some simplifications for that (attached, untested).
...
> >This patch doesn't match our coding conventions, but seems to comply with
> >the code in mod_authz_svn...
>
> Will fix mod_authz_svn itself to match our coding guidelines in
> subsequent commit.
Okay.
> >Yeah, these are really redundant; only the apr_toupper()/apr_tolower()
> >calls
> >differ. We definitely want a single function here.
> >
> >
>
> Fixed.
> >> if (conf->usernamecase) {
> >>+ username_to_authorize = apr_pstrdup(r->pool, r->user);
> >>+ if (strcasecmp(conf->usernamecase, "upper") == 0)
> >>+ convert_to_uppercase_string(username_to_authorize);
> >>+ else
> >>+ convert_to_lowercase_string(username_to_authorize);
> >>
> >
> >Ya know, I'd probably just inline the code here.
>
> Fixed.
...
> >If we don't change r->user, I think the history will take the username as
> >specified by the end-user. This means that the history will be potentially
> >subject to several capitalizations of the same name.
>
> Yes, you are correct. Can each module change the request_rec->user like
> this?
>
> Committed with above fixes in r29875
Re: [PATCH] Make mod_authz_svn to apply the authz checks against
upper/lowercased usernames
Posted by Kamesh Jayachandran <ka...@collab.net>.
Hi Dan,
> ...
>
> This sounds fairly reasonable to me. One big question I have is how usernames
> are then recorded in the history (e.g. uppercase, lowercase, or as entered
> by the user?).
>
>
Usernames are recorded as provided by the user. I did not want to change
the request_rec->user.
>> [[[
>> Make mod_authz_svn to apply the authz checks against upper/lowercased
>> usernames.
>>
>> * subversion/mod_authz_svn/mod_authz_svn.c
>> (): Include 'apr_lib.h' and 'strings.h'.
>> (struct authz_svn_config_rec): New member 'usernamecase'.
>>
>
> How about "force_username_case", or something similar that indicates what
> we're doing with username case?
>
>
Yes 'force_username_case' makes more sense. Fixed my patch.
>> (authz_svn_cmds): Populate 'authz_svn_config_rec.usernamecase'
>> from configuration directive 'AuthzUsernameCase'.
>>
>
> Ditto for the name of the directive, and even more so for the help (which
> should mention that the case is applied to authz).
>
>
Changed the directive to 'AuthzForceUsernameCase'.
>> (convert_to_uppercase_string,
>> convert_to_lowercase_string,
>> get_username_to_authorize): New functions.
>>
>
> Do we really need both of those conversion functions? Seems like one
> should be sufficient.
>
Yes fixed that too.
>
>> (req_check_access, subreq_bypass): Apply authz check against
>> upper/lowercased usernames.
>>
>
> upper/lower-cased
>
>
Fixed the log message.
> This patch doesn't match our coding conventions, but seems to comply with
> the code in mod_authz_svn...
>
>
Will fix mod_authz_svn itself to match our coding guidelines in
subsequent commit.
> Yeah, these are really redundant; only the apr_toupper()/apr_tolower() calls
> differ. We definitely want a single function here.
>
>
Fixed.
>> if (conf->usernamecase) {
>> + username_to_authorize = apr_pstrdup(r->pool, r->user);
>> + if (strcasecmp(conf->usernamecase, "upper") == 0)
>> + convert_to_uppercase_string(username_to_authorize);
>> + else
>> + convert_to_lowercase_string(username_to_authorize);
>>
>
> Ya know, I'd probably just inline the code here.
>
Fixed.
>
>> + }
>>
>
> If we don't change r->user, I think the history will take the username as
> specified by the end-user. This means that the history will be potentially
> subject to several capitalizations of the same name.
>
>
Yes, you are correct. Can each module change the request_rec->user like
this?
Committed with above fixes in r29875
Thanks for the review.
With regards
Kamesh Jayachandran
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Make mod_authz_svn to apply the authz checks against upper/lowercased usernames
Posted by "Daniel L. Rall" <dl...@finemaltcoding.com>.
On Fri, 07 Mar 2008, Kamesh Jayachandran wrote:
> People using mod_auth_sspi(windows domain authentication apache module)
> face the 'Authorization Failed' error while
> accessing SVN with mixed case usernames which does not match in *exact
> case* with their authz rules.
>
> http://blog.michaelcheng.idv.hk/ explains it in detail.
>
> mod_auth_sspi has a directive by name 'SSPIUsernameCase' with possible
> values being 'Lower/Upper'.
...
> I feel we should have similar directive in mod_authz_svn, to author the
> *sane* authz rules in these mixed
> case username scenarios.
...
This sounds fairly reasonable to me. One big question I have is how usernames
are then recorded in the history (e.g. uppercase, lowercase, or as entered
by the user?).
> [[[
> Make mod_authz_svn to apply the authz checks against upper/lowercased
> usernames.
>
> * subversion/mod_authz_svn/mod_authz_svn.c
> (): Include 'apr_lib.h' and 'strings.h'.
> (struct authz_svn_config_rec): New member 'usernamecase'.
How about "force_username_case", or something similar that indicates what
we're doing with username case?
> (authz_svn_cmds): Populate 'authz_svn_config_rec.usernamecase'
> from configuration directive 'AuthzUsernameCase'.
Ditto for the name of the directive, and even more so for the help (which
should mention that the case is applied to authz).
> (convert_to_uppercase_string,
> convert_to_lowercase_string,
> get_username_to_authorize): New functions.
Do we really need both of those conversion functions? Seems like one
should be sufficient.
> (req_check_access, subreq_bypass): Apply authz check against
> upper/lowercased usernames.
upper/lower-cased
> ]]]
This patch doesn't match our coding conventions, but seems to comply with
the code in mod_authz_svn...
> Index: subversion/mod_authz_svn/mod_authz_svn.c
> ===================================================================
> --- subversion/mod_authz_svn/mod_authz_svn.c (revision 29765)
> +++ subversion/mod_authz_svn/mod_authz_svn.c (working copy)
> @@ -28,6 +28,8 @@
> #include <ap_config.h>
> #include <ap_provider.h>
> #include <apr_uri.h>
> +#include <apr_lib.h>
> +#include <strings.h>
> #include <mod_dav.h>
>
> #include "mod_dav_svn.h"
> @@ -46,6 +48,7 @@
> int no_auth_when_anon_ok;
> const char *base_path;
> const char *access_file;
> + const char *usernamecase;
> } authz_svn_config_rec;
>
> /*
> @@ -90,6 +93,11 @@
> "Set to 'On' to suppress authentication and authorization "
> "for requests which anonymous users are allowed to perform. "
> "(default is Off.)"),
> + AP_INIT_TAKE1("AuthzUsernameCase", ap_set_string_slot,
> + (void *)APR_OFFSETOF(authz_svn_config_rec, usernamecase),
> + OR_AUTHCFG,
> + "Set to 'Upper' or 'Lower' to convert the username before "
> + "checking for authorization."),
This looks pretty good as-is.
> { NULL }
> };
>
> @@ -135,6 +143,40 @@
> return access_conf;
> }
>
> +/* Converts STR_TO_CONVERT to upper case.*/
> +static void convert_to_uppercase_string(char *str_to_convert)
> +{
> + char *c = str_to_convert;
While a temporary variable isn't strictly necessary here, it certainly doesn't
hurt readability.
> + while (*c) {
> + *c = apr_toupper(*c);
> + ++c;
> + }
> +}
> +
> +/* Converts STR_TO_CONVERT to lower case.*/
> +static void convert_to_lowercase_string(char *str_to_convert)
> +{
> + char *c = str_to_convert;
> + while (*c) {
> + *c = apr_tolower(*c);
> + ++c;
> + }
> +}
Yeah, these are really redundant; only the apr_toupper()/apr_tolower() calls
differ. We definitely want a single function here.
> +
> +static char* get_username_to_authorize(request_rec *r,
> + authz_svn_config_rec *conf)
> +{
> + char *username_to_authorize = r->user;
> + if (conf->usernamecase) {
> + username_to_authorize = apr_pstrdup(r->pool, r->user);
> + if (strcasecmp(conf->usernamecase, "upper") == 0)
> + convert_to_uppercase_string(username_to_authorize);
> + else
> + convert_to_lowercase_string(username_to_authorize);
Ya know, I'd probably just inline the code here.
> + }
If we don't change r->user, I think the history will take the username as
specified by the end-user. This means that the history will be potentially
subject to several capitalizations of the same name.
> + return username_to_authorize;
> +}
> +
> /* Check if the current request R is allowed. Upon exit *REPOS_PATH_REF
> * will contain the path and repository name that an operation was requested
> * on in the form 'name:path'. *DEST_REPOS_PATH_REF will contain the
> @@ -162,6 +204,7 @@
> svn_authz_t *access_conf = NULL;
> svn_error_t *svn_err;
> char errbuf[256];
> + const char *username_to_authorize = get_username_to_authorize(r, conf);
canonicalized_username? Dunno, username_to_authorize is a pretty good name too
(nice and explicit).
>
> switch (r->method_number) {
> /* All methods requiring read access to all subtrees of r->uri */
> @@ -307,7 +350,8 @@
> || (!repos_path && (authz_svn_type & svn_authz_write)))
> {
> svn_err = svn_repos_authz_check_access(access_conf, repos_name,
> - repos_path, r->user,
> + repos_path,
> + username_to_authorize,
> authz_svn_type,
> &authz_access_granted,
> r->pool);
> @@ -353,7 +397,7 @@
> svn_err = svn_repos_authz_check_access(access_conf,
> dest_repos_name,
> dest_repos_path,
> - r->user,
> + username_to_authorize,
> svn_authz_write
> |svn_authz_recursive,
> &authz_access_granted,
> @@ -437,9 +481,11 @@
> authz_svn_config_rec *conf = NULL;
> svn_boolean_t authz_access_granted = FALSE;
> char errbuf[256];
> + const char *username_to_authorize;
>
> conf = ap_get_module_config(r->per_dir_config,
> &authz_svn_module);
> + username_to_authorize = get_username_to_authorize(r, conf);
>
> /* If configured properly, this should never be true, but just in case. */
> if (!conf->anonymous || !conf->access_file) {
> @@ -457,7 +503,8 @@
> */
> if (repos_path) {
> svn_err = svn_repos_authz_check_access(access_conf, repos_name,
> - repos_path, r->user,
> + repos_path,
> + username_to_authorize,
> svn_authz_none|svn_authz_read,
> &authz_access_granted,
> r->pool);
Re: [PATCH] Make mod_authz_svn to apply the authz checks against
upper/lowercased usernames
Posted by Kyle George <kg...@tcpsoft.com>.
On Fri, 7 Mar 2008, Kamesh Jayachandran wrote:
> People using mod_auth_sspi(windows domain authentication apache module)
> face the 'Authorization Failed' error while accessing SVN with mixed
> case usernames which does not match in *exact case* with their authz
> rules.
>
> http://blog.michaelcheng.idv.hk/ explains it in detail.
>
> mod_auth_sspi has a directive by name 'SSPIUsernameCase' with possible
> values being 'Lower/Upper'.
>
> I am not sure whether it really uses it to compare the
> lowered/uppercased username.
>
> I feel we should have similar directive in mod_authz_svn, to author the
> *sane* authz rules in these mixed case username scenarios.
I just saw this. I didn't read your patch, but take a look at this module
I wrote that might do what you want without modifying mod_authz_svn. It
also works with more than just subversion (for cases where users often
munge their usernames). See the "AuthRewriteCase lower" directive:
http://svn.haxx.se/dev/archive-2007-04/0741.shtml
--
Kyle George
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Make mod_authz_svn to apply the authz checks against upper/lowercased usernames
Posted by "Daniel L. Rall" <dl...@finemaltcoding.com>.
On Thu, 13 Mar 2008, David Glasser wrote:
> 2008/3/7 Kamesh Jayachandran <ka...@collab.net>:
...
> > +static char* get_username_to_authorize(request_rec *r,
> > + authz_svn_config_rec *conf)
> > +{
> > + char *username_to_authorize = r->user;
> > + if (conf->usernamecase) {
> > + username_to_authorize = apr_pstrdup(r->pool, r->user);
> > + if (strcasecmp(conf->usernamecase, "upper") == 0)
> > + convert_to_uppercase_string(username_to_authorize);
> > + else
> > + convert_to_lowercase_string(username_to_authorize);
>
> Maybe check (perhaps somewhere else) that the given option is really
> upper or lower? Silently accepting, say, "uppercase" as "lower" can
> be confusing.
I had the same thought when looking through this originally, but forgot to
mention it.
Garbage values shouldn't perform any conversion, and should log an error.
Or, cause a config file parse failure.
Re: [PATCH] Make mod_authz_svn to apply the authz checks against upper/lowercased usernames
Posted by David Glasser <gl...@davidglasser.net>.
2008/3/7 Kamesh Jayachandran <ka...@collab.net>:
> Hi All,
>
> People using mod_auth_sspi(windows domain authentication apache module)
> face the 'Authorization Failed' error while
> accessing SVN with mixed case usernames which does not match in *exact
> case* with their authz rules.
>
> http://blog.michaelcheng.idv.hk/ explains it in detail.
>
> mod_auth_sspi has a directive by name 'SSPIUsernameCase' with possible
> values being 'Lower/Upper'.
>
> I am not sure whether it really uses it to compare the
> lowered/uppercased username.
>
> I feel we should have similar directive in mod_authz_svn, to author the
> *sane* authz rules in these mixed
> case username scenarios.
>
> Attached patch accomplishes that.
>
> Want to know what community thinks about it.
>
> With regards
> Kamesh Jayachandran
>
> [[[
> Make mod_authz_svn to apply the authz checks against upper/lowercased
> usernames.
>
> * subversion/mod_authz_svn/mod_authz_svn.c
> (): Include 'apr_lib.h' and 'strings.h'.
> (struct authz_svn_config_rec): New member 'usernamecase'.
> (authz_svn_cmds): Populate 'authz_svn_config_rec.usernamecase'
> from configuration directive 'AuthzUsernameCase'.
> (convert_to_uppercase_string,
> convert_to_lowercase_string,
> get_username_to_authorize): New functions.
> (req_check_access, subreq_bypass): Apply authz check against
> upper/lowercased usernames.
> ]]]
>
> Index: subversion/mod_authz_svn/mod_authz_svn.c
> ===================================================================
> +static char* get_username_to_authorize(request_rec *r,
> + authz_svn_config_rec *conf)
> +{
> + char *username_to_authorize = r->user;
> + if (conf->usernamecase) {
> + username_to_authorize = apr_pstrdup(r->pool, r->user);
> + if (strcasecmp(conf->usernamecase, "upper") == 0)
> + convert_to_uppercase_string(username_to_authorize);
> + else
> + convert_to_lowercase_string(username_to_authorize);
Maybe check (perhaps somewhere else) that the given option is really
upper or lower? Silently accepting, say, "uppercase" as "lower" can
be confusing.
--dave
--
David Glasser | glasser@davidglasser.net | http://www.davidglasser.net/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org