Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Mixed case username auth not working anymore #56

Open
jmwf opened this issue Sep 18, 2018 · 14 comments
Open

Mixed case username auth not working anymore #56

jmwf opened this issue Sep 18, 2018 · 14 comments

Comments

@jmwf
Copy link

jmwf commented Sep 18, 2018

First off, thanks you for updating this great tool!

We have been using WebSVN for a number of years and stayed happily with 2.3.3 until last week.
The new version (2.4) works great in supporting downloads and comparison in a PHP 7+ environment, but one thing that has changed is the way that WebSVN connects to subversion.

In our apache/subversion setup we use
AuthzForceUsernameCase Lower

The old WebSVN (2.3.3) worked very well with this setting, but WebSVN 2.4 require users to login with their username in lowercase to match the configuration used in the subversion authz-file. While it is not really a big deal, it still probably affects a lot of installations.
I don't know php-programming, but comparing the old auth.php and the new authz.php indicates that the old 2.3.3 version used strtolower() on the user name, but the new version does not?

My suggestion would be to add a new configuration parameter to let the user decide if WebSVN should force lowercase/uppercase or not.

/J

@michael-o
Copy link
Member

So as far as I understand the issue, $REMOTE_USER differs from the value the mod_authz_svn is using to query the authz file? If so, I'd rather set an env variable with the same value as with AuthzForceUsernameCase which will be automatically read by authz.php. You could also rewrite the remote user with a directive.

@jmwf
Copy link
Author

jmwf commented Sep 18, 2018

As I said, I really don't know much about PHP programming, I'm only seeing this as a change to how it used to work. We actually have both 2.3.3 and 2.4 running side-by-side at the moment, and they behave differently.

I'm not clear what you mean by "set an env variable with the same value as with AuthzForceUsernameCase".
Would this mean that I can configure WebSVN (in config.php) to force Lowercase/Uppercase?

/J

@michael-o
Copy link
Member

I'm not clear what you mean by "set an env variable with the same value as with AuthzForceUsernameCase".
Would this mean that I can configure WebSVN (in config.php) to force Lowercase/Uppercase?

If set in httpd.conf, there won't be a need for a config option in config.php. Alternatively, you could modify REMOTE_USER yourself to achieve the goal. Have you evaluated this?

@jmwf
Copy link
Author

jmwf commented Sep 18, 2018

I have not evaluated changing httpd.conf, basically because I think this is application specific, not apache specific. I've thought about changing this in the setUsername() function, but would like it to be configurable in the config.php to avoid future work when new WebSVN updates are installed.

For now I will tell my users to login with lowercase names as that is the most safe route at the moment.
/J

@ams-tschoening
Copy link
Contributor

Maybe it's clear for others, it wasn't for me, so feel free to ignore the following or correct me if I'm wrong:

AuthzForceUsernameCase Lower

That setting was not used by WebSVN itself, correct? It was somewhere in the httpd configuration files itself, because like @michael-o linked already, it seems to be only used by mod_auth_svn. As @jmwf already mentioned, WebSVN simply used hard-coded strtolower in its custom implementation at various places, not only for usernames, but paths as well. Read the following comment:

// .ini parser converts groups to lower-case

I have the feeling that lower-case was simply used at all/some(?) places because some of those needed to be lower anyway to work at all because of the mentioned limitations of the parser. WebSVN did lower-case only as well, while AuthzForceUsernameCase is able to support lower and upper. @michael-o replaced the custom parser with some official implementation and that doesn't seem to have some setting like AuthzForceUsernameCase at all.

In my opinion, what is done now is the correct behaviour, case should be significant by default if SVN decides so, but it's incompatible with former releases as well. The problem I see now is that this isn't only incompatible for usernames, but repo names and paths as well.

So, should both incompatibilities be addressed? Or only usernames and if so, how?

In my opinion again we should only care about usernames if at all, because mod_authz_svn only cares about those. Additionally, it would be great if one would be able to directly access AuthzForceUsernameCase, but I haven't found any way to do so in PHP. This would leave us with some custom setting, maybe simply using the same name. :-)

Let's compare how complex it is to fix this using httpd, like @michael-o mentioned. For URIs:

RewriteEngine On
RewriteMap  lc int:tolower
RewriteCond %{REQUEST_URI} ([a-zA-Z0-9]+):([a-zA-Z0-9]+)@svn.domain.com(.*) [OR]
RewriteCond %{REQUEST_URI} svn.domain.com/(.+)
RewriteRule (.*) ${lc:$1} [R=301,L]

https://stackoverflow.com/a/8308541/2055163

Which doesn't work if browsers ask for usernames, in such a case it seems one would need to rewrite the environment variable directly?

RewriteEngine On
RewriteMap  lc int:tolower
RewriteRule ^ - [E=REMOTE_USER:${lc:%{ENV:REMOTE_USER}},L]

https://stackoverflow.com/a/18476427/2055163

Doesn't look that wrong, but probably more difficult than to only change one setting in WebSVN. OTOH, the same approach could be used for usernames, paths, repo names etc. as well, everything that is now different than before and we might simply not want to change anymore.

Thoughts?

@michael-o
Copy link
Member

michael-o commented Sep 18, 2018

I would not touch repo and path names because they are case sensitive. URIs are always case-sensitive.

This looks fine to me:

$ grep -r strtolower .
./filedetails.php:      $extn = strtolower(strrchr($path, '.'));
./include/utils.php:    } else if (isset($_SERVER['HTTPS']) && (strtolower($_SERVER['HTTPS']) != 'off')) {
./listing.php:                                  $listvar['filetype'] = strtolower(strrchr($file, '.'));
./log.php:      $words[$index] = strtolower(removeAccents($word));
./log.php:                                              if (strpos(strtolower(removeAccents($revision->msg)), $word) === false && strpos(strtolower(removeAccents($revision->author)), $word) === false) {

I still think that this is an implemenation detail performed by the subversion module and it should be done by the httpd.conf as well.

We could allow a SVN_REMOTE_USER with the lower/upper version of REMOTE_USER and read it in here with the rewrite @ams-tschoening is proposing. A two-line change for us.

@jmwf
Copy link
Author

jmwf commented Sep 18, 2018

I agree that repo names and path names should not be touched, that is up to the server to handle.
But usernames are more tricky in the subversion authz file , and this is the reason I guess mod_dav_svn implements "AuthzForceUsernameCase".

We authenticate our users against an OpenLDAP server, and the users can login with User, uSer, user, USER ... which means that authz must cover all these combinations. Unless we use "AuthzForceUsernameCase Lower" to only have to write each user once.

In my view WebSVN is kind of a frontend to subversion and should preferably work the same way.
Another problem I see is that WebSVN could actually access more than one repo (or parent repo folders), so in theory they could actually use different settings depending on the subversion server.

/J
PS I will try the rewrite rule for the REMOTE_USER

@ams-tschoening
Copy link
Contributor

Another problem I see is that WebSVN could actually access more than one repo (or parent repo folders), so in theory they could actually use different settings depending on the subversion server.

Interesting point, but can be handled easily: It didn't work that way before and therefore won't be introduced newly. ;-) If at all, this issue is about re-introducing reasonable backwards compatibility for usernames. Your argument seems to be a good point for "forcing" admins to deal with those specifics on the level of the webserver anyway, because they have full control for all requests there.

We could allow a SVN_REMOTE_USER with the lower/upper version of REMOTE_USER and read it in here with the rewrite @ams-tschoening is proposing. A two-line change for us.

Not that I'm voting against this, but we should keep in mind that there's more than Apache httpd out there. Everyone would need to reimplement at least the username thing, which doesn't seem that uncommon according to my quick research.

@jmwf
Copy link
Author

jmwf commented Sep 18, 2018

Ideally I'd like to see an an additional option in the config.php parts, e.g.
$config->useAccessFile('/path/to/accessfile', 'lowercase'); // set access file using lowercase usernames
$config->useAccessFile('/path/to/accessfile', 'uppercase'); // set access file using uppercase usernames

or something similar. That way each repo can be controlled separately.
/J

@michael-o
Copy link
Member

@jmwf This could work. Though still ugly, because I think that username normalization is a auth provider issue. I see the directory in the module as a compromise too. @ams-tschoening WDYT? Is that a way we can go?

@jmwf
Copy link
Author

jmwf commented Sep 19, 2018

One problem I think is that WebSVN can use URI in the form "file://" to access subversion repositories for performance, and in this case subversion doesn't handle authz at all (which I think is the reason for the $config->useAccessFile in the first place). The authz file is only relevant if served through svnserve, apache etc.

/J

@ams-tschoening
Copy link
Contributor

@jmwf I don't get your last point, WebSVN internally uses svnauthz accessof and doesn't rely on svnserve or httpd or such.

@michael-o I think adding it there makes sense, but we need to be careful about the syntax, because an additional optional argument to provide a repo name is already supported:

$config->useAccessFile('/path/to/accessfile');
$config->useAccessFile('/path/to/accessfile', 'myrep');
function useAccessFile($file, $myrep = 0) {

I'm using that myself as well. So we would at least need something like:

$config->useAccessFile('/path/to/accessfile', null, 'lowercase');
$config->useAccessFile('/path/to/accessfile', 'myrep', 'uppercase');

Or introduce some hash-based settings? Not sure if it works this way in PHP:

$config->useAccessFile('/path/to/accessfile', 'repo' => 'myrep', '[authzF|f]orceUserNameCase' => 'lower');

All functions processing these calls could easily forward "lowercase" to authz.php and that could recognize it.

@jmwf
Copy link
Author

jmwf commented Sep 19, 2018

It probably misunderstood how WebSVN accesses the repositories :-(
I thought that if the repository was remote and accessed by for example https://...., the access was limited by the remote servers´ authz file.
For access to a local repository by file paths, subversion doesn't have any authz checks done, hence the need for it to happen in WebSVN for local repository access..

/J

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants