GUACAMOLE-1239: Make identifier comparison case-insensitive.

[necouchman]

This is an attempt to make comparison of usernames case-insensitive, as most authentication systems do not distinguish between case in usernames. This goes about it by 1) converting all identifiers to lower-case, and 2) making the comparison of identifiers case-insensitive.

I'm really more doing this to get the conversation started on the best way to go about this. I realize that this method is a sort of atomic bomb approach - it's simple, but has a lot of (potentially negative) implications. It's also not in any way configurable. I'm happy to discuss other ways that we can accomplish it, but both personal experience with case-sensitive usernames in Guacamole along with the other folks who are asking for it makes me believe it's something we need to address.

[mike-jumper]

This looks reasonable to me, though I think it should be something that's determined by the implementation.

What about adding a function like isIdentifierCaseSensitive() to Identifiable, with a default implementation that just returns true? The equals() comparison could then take that into account (hashCode() would need to do the same).

The other issue will be with database queries. IIRC, MySQL/MariaDB is case-insensitive for username queries, but PostgreSQL is case-sensitive. We would need to somehow allow for MySQL/MariaDB to optionally consider case and for PostgreSQL to optionally ignore case ... ideally without making any schema changes.

[necouchman]

The other issue will be with database queries. IIRC, MySQL/MariaDB is case-insensitive for username queries, but PostgreSQL is case-sensitive. We would need to somehow allow for MySQL/MariaDB to optionally consider case and for PostgreSQL to optionally ignore case ... ideally without making any schema changes.

Are you talking about the actual DB usernames (as in what would be configured in guacamole.properties), or just the normal string-based queries in the tables? These changes, here, shouldn't actually impact the Postgres (or MySQL or MSSQL) usernames and/or passwords that Guacamole uses to access the database, and I would think that all databases consider case when doing comparisons for varchar fields, like the name field in the guacamole_entity table?

[mike-jumper]

I'm referring to comparisons against the guacamole_entity table. There does seem to be a difference in how basic string comparisons are performed between the databases, probably due to differing defaults in collation.

When I try things against PostgreSQL, guacadmin and GUACADMIN are different:

guacamole_db=# SELECT * FROM guacamole_entity WHERE type = 'USER' AND name = 'guacadmin';
 entity_id |   name    | type 
-----------+-----------+------
         1 | guacadmin | USER
(1 row)

guacamole_db=# SELECT * FROM guacamole_entity WHERE type = 'USER' AND name = 'GUACADMIN';
 entity_id | name | type 
-----------+------+------
(0 rows)

When I try against MySQL, they're the same:

MariaDB [guacamole_db]> SELECT * FROM guacamole_entity WHERE type = 'USER' AND name = 'guacadmin';
+-----------+-----------+------+
| entity_id | name      | type |
+-----------+-----------+------+
|         1 | guacadmin | USER |
+-----------+-----------+------+
1 row in set (0.000 sec)

MariaDB [guacamole_db]> SELECT * FROM guacamole_entity WHERE type = 'USER' AND name = 'GUACADMIN';
+-----------+-----------+------+
| entity_id | name      | type |
+-----------+-----------+------+
|         1 | guacadmin | USER |
+-----------+-----------+------+
1 row in set (0.001 sec)

[necouchman]

Got it - I'll take a run at it.

[necouchman]

The generally accepted method for PostgreSQL to do case-insensitive searches/comparisons seems to be to use the lower() function to convert both strings to lower-case before the comparison.

MySQL appears to have a couple of options:

• Create or edit collation of the column in question to a case-sensitive option. The downside of this is that the schema has to either be changed or modified on-the-fly.
• Use a binary option when doing the column comparison, which might have a performance hit - views on that are mixed - because it may bypass indices.

[necouchman]

The other issue will be with database queries. IIRC, MySQL/MariaDB is case-insensitive for username queries, but PostgreSQL is case-sensitive. We would need to somehow allow for MySQL/MariaDB to optionally consider case and for PostgreSQL to optionally ignore case ... ideally without making any schema changes.

@mike-jumper : So I take it you're thinking that this should be configurable - default to case-sensitive, perhaps, but with a guacamole.properties option to make it case-insensitive?

[mike-jumper]

I think the ideal would be a bit of a combination of all worlds:

• Some mechanism for an implementation to declare whether its identifiers are case-sensitive. If an implementation is absolutely inherently case-insensitive (like LDAP) or case-sensitive (like local Linux accounts), it would make sense for that fact to be made available to other implementations consuming those identifiers.
• Configuration options for any implementations where case sensitivity is within implementation control and it makes sense to provide such an option (I'd think the database backends would fall into this category).

Simply exposing whether identifiers are intended to be treated as case-sensitive or case-insensitive should already bridge a large gap. For example, if a user authenticates using a case-insensitive account via LDAP, a case-sensitive backend like PostgreSQL could intelligently and automatically choose to transform that identifier into a format that would not be affected by case sensitivity (lowercase? uppercase?).

[necouchman]

Converted to a draft for now - I think I've accomplished just about everything except the ability to force the JDBC modules to deal with usernames in one form or the other Testing so far shows that an LDAP username entered in a different case will 1) get permissions assigned in JDBC from the username regardless of case, and 2) will not create duplicate user entries auto-created in a Postgres database.

[necouchman]

@mike-jumper

I'm working through the JDBC changes required to make case sensitivity configurable, and I'm getting a bit confused in some of the mapper files. For example:

guacamole-client/extensions/guacamole-auth-jdbc/modules/guacamole-auth-jdbc-postgresql/src/main/resources/org/apache/guacamole/auth/jdbc/permission/UserPermissionMapper.xml

Lines 26 to 32 in 3611be5

	<!-- Result mapper for user permissions -->
	<resultMap id="UserPermissionResultMap" type="org.apache.guacamole.auth.jdbc.permission.ObjectPermissionModel">
	<result column="entity_id" property="entityID" jdbcType="INTEGER"/>
	<result column="permission" property="type" jdbcType="VARCHAR"
	javaType="org.apache.guacamole.net.auth.permission.ObjectPermission$Type"/>
	<result column="affected_name" property="objectIdentifier" jdbcType="INTEGER"/>
	</resultMap>

The affected_name column is labeled as a _name, not _identifier; however, it maps to the objectIdentifier variable and has a type of INTEGER. If I look at the ObjectPermissionModel class, though:

guacamole-client/extensions/guacamole-auth-jdbc/modules/guacamole-auth-jdbc-base/src/main/java/org/apache/guacamole/auth/jdbc/permission/ObjectPermissionModel.java

Lines 30 to 33 in 3611be5

	/**
	* The unique identifier of the object affected by this permission.
	*/
	private String objectIdentifier;

objectIdentifier has a type of String. So, shouldn't the jdbcType in the mapper file be VARCHAR, and not INTEGER?

== EDIT
After spending some more time looking at the UserPermissionMapper.xml file, it seems that maybe the term objectIdentifier is the "name" representation of the object (username or group name), rather than the database user_id or group_id? Maybe this is the source of most of my confusion; although, it still seems like the type of the affect_name column in the result map should be VARCHAR and not INTEGER?

Or am I missing something else, here?

[mike-jumper]

Here, since the relevant column from the database result is affected_name and that is indeed a string, the jdbcType here should also be VARCHAR. This probably only works as-is because:

• The databases we support and their drivers are forgiving in this area.
• According to the MyBatis documentation, the jdbcType attribute is only required for nullable columns during INSERT/UPDATE/DELETE. BUT, since result maps are only relevant to SELECT queries, this seems a rather long-winded way of stating that the declared jdbcType has no impact on result maps.

I can't find any examples in the MyBatis documentation that show jdbcType ever being used for any <result>, so this may well be unnecessary.

You're correct that absolutely all of the Identifiable objects within the JDBC auth use identifiers that happen to be integers, with the exceptions of users and groups which instead use the username / group name. The integer IDs of users and groups are used only internally.

[necouchman]

@mike-jumper @jmuehlner

Appreciate any feedback you have on this most recent round of changes. I've spent some time trying to re-work the various JDBC modules to handle configurable case sensitivity settings. For each of the databases:

• PostgreSQL is case-sensitive by default - the widely-accepted way to make it case-insensitive is to convert everything to either lower or upper-case when comparing. I opted for LOWER().
• MySQL is case-insensitive by default, and the community seems quite split over the best way to accomplish case-sensitive searches. The easiest way, and the way I opted to go, is to use the BINARY keyword in the WHERE and JOIN clauses that need to be converted to case-sensitive comparisons. The better way is to change the collation used in the queries; however, the challenge, here, is that it requires some knowledge of what collation is being used to begin with, which (I don't think) can be programmatically detected in a way that we can use. Maybe I'm wrong, and there is a way to do this?? The BINARY method has two pitfalls that I read about: 1) it ends up bypassing indexing, so there are some other creative things you have to do to insure you make use of that, and 2) it has some potential pitfalls when double-byte character sets are in use.
• SQLServer - haven't even dealt with this one, yet - figured I'd get consensus on how to handle Postgres and MySQL before tackling Microsoft. From what I've read, the only way to do this is to change the collation during query time, which is going to have the same challenge as that route with MySQL - you have to know something of what collation is in use to begin with.

Aside from database-specific issues, the other big thing to look at is how I've actually changed the interfaces that MyBatis uses with its mapper files to generate the required code...

• I was hoping to come up with some way to use something within the XML mapper files that could query the injected JDBCEnvironment object and call the getCaseSensitiveUsernames() method without having to modify the arguments of any of the queries. I could not figure out a way - if I've overlooked it, PLEASE tell me so that I can clean this up.
• Having not been able to figure out a way to do injection into the XML mapper files, I went with just modifying the arguments of the classes that need to know about the case-sensitivity, which leans to the side of breaking interface inheritance over passing useless parameters to methods that don't need it.
• The other thing to consider would be if we want to (continue to) limit the case sensitivity setting to only usernames (as I've done, here), or if we want to make it where we're configuring all string comparison within a given DB environment (usernames, group names, connection names...whatever strings get compared). Or somewhere in between.

[jmuehlner]

The better way is to change the collation used in the queries; however, the challenge, here, is that it requires some knowledge of what collation is being used to begin with, which (I don't think) can be programmatically detected in a way that we can use. Maybe I'm wrong, and there is a way to do this??

You should be able to query which collations are available on a database using SHOW COLLATION, but yeah it sounds like there's differing collations available in various version of MySQL/MariaDB. Trying to build something that queries which is available and tries to find the best one sounds like a mess.

So short answer no I don't know of a better way, sadly.

This is all more of a pain than I would have ever guessed, but generally your approach looks pretty reasonable to me so far.

[necouchman]

The better way is to change the collation used in the queries; however, the challenge, here, is that it requires some knowledge of what collation is being used to begin with, which (I don't think) can be programmatically detected in a way that we can use. Maybe I'm wrong, and there is a way to do this??

You should be able to query which collations are available on a database using SHOW COLLATION, but yeah it sounds like there's differing collations available in various version of MySQL/MariaDB. Trying to build something that queries which is available and tries to find the best one sounds like a mess.

So short answer no I don't know of a better way, sadly.

This is all more of a pain than I would have ever guessed, but generally your approach looks pretty reasonable to me so far.

Thanks for the feedback. Yeah, it seems like, even using the SHOW COLLATION option, we'd have to have some sort of look-up table that said "if collation is x, switch to y", but that means maintaining a lookup table...

[necouchman]

I suppose the other route to consider, here, is to make the JDBC changes specific to forcing the PostgreSQL module into case-insensitive mode, and not worry about the settings/options/changes that force MySQL and/or SQL Server into case-sensitive mode? It seems like the most frequent request is to make the look-ups and comparisons case-_in_sensitive, and since MySQL and SQL Server already do that by default, we could just go with that.

@mike-jumper @jmuehlner Further thoughts?

[necouchman]

@jmuehlner or @mike-jumper Ping on this one, so I can either clean it up for 1.6.0, or de-scope it if we need to. I'd rather it make it into this release if at all possible...

[jmuehlner]

@jmuehlner or @mike-jumper Ping on this one, so I can either clean it up for 1.6.0, or de-scope it if we need to. I'd rather it make it into this release if at all possible...

I'm fine with the idea of offering an option to enable case-insensitive comparison, and just let it use the default case sensitiy behavior of the database otherwise. I think this would cover 95% of the use case of this feature.

I'm fine with including this in 1.6.0 if it's ready.

[necouchman]

Okay, I've gone through and tried to make sure comments are consistent. I think it's fixed up. I also believe I got the injection of the SSOEnvironment stuff correct in the SSO modules, but I still need to test, and it might be worth someone having a closer look at that.

I also had another thought - I've done all of this work on a per-module basis, creating configuration options, etc. Does it make sense, instead, to do something like:

• Global option for case-sensitivity across the entire Guacamole installation, instead of per-extension (case-sensitive-usernames:)?
• Make the check for case sensitivity part of the Environment class, so that anything/everything that has access to that class can pull the status.
• If username case-sensitivity is enabled, it is enabled across the board, for all the extensions; if it's disabled, it's disabled across the board for all extensions.

[jmuehlner]

Global option for case-sensitivity across the entire Guacamole installation, instead of per-extension (case-sensitive-usernames:)?

Eh, I'm not sure. Would there be situations potentially where somebody might want to configure the application to do case-insensitive comparisons for some extensions, but not others? If so we wouldn't want to prevent them from configuring it that way.

I could see how having a global option would be really handy, but maybe it could be overriden on a per-extension basis?

[necouchman]

Global option for case-sensitivity across the entire Guacamole installation, instead of per-extension (case-sensitive-usernames:)?

Eh, I'm not sure. Would there be situations potentially where somebody might want to configure the application to do case-insensitive comparisons for some extensions, but not others? If so we wouldn't want to prevent them from configuring it that way.

I could see how having a global option would be really handy, but maybe it could be overriden on a per-extension basis?

Yeah, that's why I was asking. I wasn't sure if we could come up with scenarios one way or the other - it seems like, most of the time, the request is going to be to make it behave one way or the other across the board, particularly between various extensions (JDBC + LDAP, for example). But always good to have the flexibility there to let folks do what they like with it. I'll give it a go - I have an idea, and I think it'll be reasonably easy to do...

[necouchman]

Okay, I think I've got things fixed up. I have some testing to do, yet, but both global and per-extension options exist, now, with the per-extension options overriding the global option.

[jmuehlner]

Okay, I think I've got things fixed up. I have some testing to do, yet, but both global and per-extension options exist, now, with the per-extension options overriding the global option.

Looks good to me! Any further concerns, @mike-jumper?

[mike-jumper]

Looks pretty good to me. I only found one issue with property naming consistency that I think is worth addressing.