GUACAMOLE-1239: Correct logic for, and fix missing, case sensitivity settings.

[necouchman]

Next round of fixes for case-sensitivity stuff in the JDBC module.

Opening as a draft for the moment while I verify that the changes I've made will actually work consistently for username case-sensitivity.

[corentin-soriano]

@necouchman I found another issue related to the case sensitive with the quick connect extension.
It is still present on your branch:

Oct 14 08:28:46 guacamole.host server[3886265]: 08:28:46.647 [http-nio-8080-exec-4] DEBUG o.a.g.event.EventLoggingListener - User "corentin" (authenticated by "openid") successfully accessed/retrieved connection "0" within "quickconnect" (currently named "ssh://127.0.0.1/")
Oct 14 08:28:46 guacamole.host server[3886265]: 14-Oct-2024 08:28:46.852 SEVERE [http-nio-8080-exec-14] org.apache.coyote.AbstractProtocol$ConnectionHandler.process Error reading request, ignored
Oct 14 08:28:46 guacamole.host server[3886265]:         java.lang.NullPointerException
Oct 14 08:28:46 guacamole.host server[3886265]:                 at org.apache.guacamole.auth.jdbc.HistoryTrackingConnection.connect(HistoryTrackingConnection.java:109)
Oct 14 08:28:46 guacamole.host server[3886265]:                 at org.apache.guacamole.net.auth.DelegatingConnection.connect(DelegatingConnection.java:160)
Oct 14 08:28:46 guacamole.host server[3886265]:                 at org.apache.guacamole.net.auth.DelegatingConnection.connect(DelegatingConnection.java:171)
Oct 14 08:28:46 guacamole.host server[3886265]:                 at org.apache.guacamole.net.auth.TokenInjectingConnection.connect(TokenInjectingConnection.java:115)
Oct 14 08:28:46 guacamole.host server[3886265]:                 at org.apache.guacamole.net.auth.DelegatingConnection.connect(DelegatingConnection.java:160)
Oct 14 08:28:46 guacamole.host server[3886265]:                 at org.apache.guacamole.net.auth.DelegatingConnection.connect(DelegatingConnection.java:171)
Oct 14 08:28:46 guacamole.host server[3886265]:                 at org.apache.guacamole.tunnel.TunnelRequestService.createConnectedTunnel(TunnelRequestService.java:216)
Oct 14 08:28:46 guacamole.host server[3886265]:                 at org.apache.guacamole.tunnel.TunnelRequestService.createTunnel(TunnelRequestService.java:352)
Oct 14 08:28:46 guacamole.host server[3886265]:                 at org.apache.guacamole.tunnel.websocket.RestrictedGuacamoleWebSocketTunnelEndpoint.createTunnel(RestrictedGuacamoleWebSocketTunnelEndpoint.java:113)
Oct 14 08:28:46 guacamole.host server[3886265]:                 at org.apache.guacamole.websocket.GuacamoleWebSocketTunnelEndpoint.onOpen(GuacamoleWebSocketTunnelEndpoint.java:200)
Oct 14 08:28:46 guacamole.host server[3886265]:                 at org.apache.tomcat.websocket.server.WsHttpUpgradeHandler.init(WsHttpUpgradeHandler.java:131)
Oct 14 08:28:46 guacamole.host server[3886265]:                 at org.apache.coyote.AbstractProtocol$ConnectionHandler.process(AbstractProtocol.java:970)
Oct 14 08:28:46 guacamole.host server[3886265]:                 at org.apache.tomcat.util.net.NioEndpoint$SocketProcessor.doRun(NioEndpoint.java:1786)
Oct 14 08:28:46 guacamole.host server[3886265]:                 at org.apache.tomcat.util.net.SocketProcessorBase.run(SocketProcessorBase.java:52)
Oct 14 08:28:46 guacamole.host server[3886265]:                 at org.apache.tomcat.util.threads.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1191)
Oct 14 08:28:46 guacamole.host server[3886265]:                 at org.apache.tomcat.util.threads.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:659)
Oct 14 08:28:46 guacamole.host server[3886265]:                 at org.apache.tomcat.util.threads.TaskThread$WrappingRunnable.run(TaskThread.java:63)
Oct 14 08:28:46 guacamole.host server[3886265]:                 at java.base/java.lang.Thread.run(Thread.java:829)
Oct 14 08:28:46 guacamole.host server[3886265]: 08:28:46.989 [http-nio-8080-exec-10] WARN  o.a.g.a.mysql.conf.MySQLEnvironment - You have enabled case-sensitive usernames; however, MySQL's default collations do not support case-sensitive string comparisons. If you really want case-sensitive usernames you will need to configure your database appropriately.

HistoryTrackingConnection.java:109

        connectionRecordMapper.insert(connectionRecordModel,
                environment.getCaseSensitiveUsernames());

The extension works on an instance without auth-jdbc.
Can you take a look please?

[necouchman]

@corentin-soriano It looks like maybe you're not using the latest git staging/1.6.0 version after the merge of the latest round of changes? Specifically, the warning about having enabled case-sensitive usernames was moved so that it only occurs during module loading, and not at each call, so you shouldn't have that warning right below the connection attempt, which indicates you may have an older version. I'll still test it out and see what happens with that extension in my environment.

Thank you for the continued testing and reporting these issues!!

***UPDATE: I'm able to reproduce the issues noted above, so I'll try to track down why this is happening...

[necouchman]

@corentin-soriano Okay, I've corrected the latest issue you identified with non-JDBC connections inserting history records into the database in this pull request.

@mike-jumper You might take a look and make sure that what I did is sane - I couldn't pull the JDBCEnvironment injection into the HistoryTrackingConnection class, so I ended up using the isCaseSensitive() method from the currentUser. I think this is sane, but let me know if anything seems off about that...

I've also tested out things like creating users, creating connections, assigning permissions, etc., and I'm not seeing any issues, so I'm going to mark this as ready to review. I'll continue testing a few other scenarios - enabling/disabling case sensitivity, add some usernames with mixed case, etc., and see if I can break anything else.

[corentin-soriano]

@necouchman I no longer encounter the issue, thank you!

[jmuehlner]

@mike-jumper You might take a look and make sure that what I did is sane - I couldn't pull the JDBCEnvironment injection into the HistoryTrackingConnection class, so I ended up using the isCaseSensitive() method from the currentUser. I think this is sane, but let me know if anything seems off about that...

It looks like this should work, but it's a little goofy to be reaching through the user to get access to the environment. Consider injecting the environment into the UserContext (and so forth for the connection), which you can do by using a factory when instead of calling the constructor directly.

[mike-jumper]

I think it could potentially be problematic to leverage isCaseSensitive() in this way, since the meaning of isCaseSensitive() ("is the identifier of this object case sensitive?") is different from the meaning of the configuration property ("should identifiers be considered case sensitive?"), even if their values will usually be the same.

[necouchman]

Thanks @jmuehlner and @mike-jumper - a question comes to mind before I pursue a particular solution: In these situations, which environment should the case-(in)sensitivity be pulled from? Since the HistoryTrackingConnection is decorating another Connection via the HistoryTrackingUserContext (in the case of Corentin's example, the QuickConnect user context and connections), should the case sensitivity be pulled from the 1) the JDBC extension, 2) the QuickConnect extension, or 3) the global setting? My inclination is to say 2 - it should come from the decorated class(es)/extension, and not from the decorating extension, but curious what you think?

[mike-jumper]

The current approach with the various *-case-sensitive-usernames properties is that the property dictates how the relevant extension handles username comparisons, regardless of how the authenticating extension may handle username comparisons. I think the only way to be consistent with that logic would be for HistoryTrackingConnection to honor only the value from the *-case-sensitive-usernames property of the extension that defines that instance of HistoryTrackingConnection.

I also think it would make sense to allow the authenticating extension to dictate how the identifiers it presents should be compared (isCaseSensitive()), but that would be different semantics from what we currently have here.

To switch over to that approach, the other cases where case sensitivity is handled would need to be updated to honor isCaseSensitive() instead of relying purely on the configuration property, and care would need to be taken to make sure a case-insensitive authentication provider can't be used to escalate privileges (for example: by creating an unprivileged GuAcAdMiN user in some case-insensitive auth system, logging in as that user, and inheriting the permissions of guacadmin).

The current approach is less automatic, but I think that's a Good Thing, since any change from the default, strict behavior must be explicitly requested by the admin.

[necouchman]

The current approach with the various *-case-sensitive-usernames properties is that the property dictates how the relevant extension handles username comparisons, regardless of how the authenticating extension may handle username comparisons. I think the only way to be consistent with that logic would be for HistoryTrackingConnection to honor only the value from the *-case-sensitive-usernames property of the extension that defines that instance of HistoryTrackingConnection.

So, if I'm understanding what you're saying correctly, given the current way these properties are interpreted, HistoryTrackingConnection should pull the value from the JDBCEnvironment?

I also think it would make sense to allow the authenticating extension to dictate how the identifiers it presents should be compared (isCaseSensitive()), but that would be different semantics from what we currently have here.

To switch over to that approach, the other cases where case sensitivity is handled would need to be updated to honor isCaseSensitive() instead of relying purely on the configuration property, and care would need to be taken to make sure a case-insensitive authentication provider can't be used to escalate privileges (for example: by creating an unprivileged GuAcAdMiN user in some case-insensitive auth system, logging in as that user, and inheriting the permissions of guacadmin).

The current approach is less automatic, but I think that's a Good Thing, since any change from the default, strict behavior must be explicitly requested by the admin.

Yeah, seems like this might be a future enhancement with a lot more thought and effort putting into doing it safely and with as little surprise as possible.

[necouchman]

The current approach with the various *-case-sensitive-usernames properties is that the property dictates how the relevant extension handles username comparisons, regardless of how the authenticating extension may handle username comparisons. I think the only way to be consistent with that logic would be for HistoryTrackingConnection to honor only the value from the *-case-sensitive-usernames property of the extension that defines that instance of HistoryTrackingConnection.

So, if I'm understanding what you're saying correctly, given the current way these properties are interpreted, HistoryTrackingConnection should pull the value from the JDBCEnvironment?

Actually, maybe I'm just confused, here. Given Corentin's example:

• guacamole-auth-jdbc is the Authenticating extension.
• guacamole-auth-quickconnect is defining the Connection that is ultimately wrapped inside the HistoryTrackingConnection

So, you're saying that the extension that defines the Connection - quickconnect in this case - would be the one that defines the case-sensitivity for that particular wrapped object, regardless of the jdbc extension's setting?

[mike-jumper]

Hm ... maybe what I was thinking is actually backwards, and we should instead:

• Use the *-case-sensitive-usernames properties to dictate only whether users authenticating via that extension have case-sensitive usernames.
• Extensions that consume usernames from other extensions should honor the isCaseSensitive() value when comparing identities.
• In the equality comparison for AbstractIdentifiable, we need some sane way to resolve what happens if you compare a case-sensitive identifier with a case-insensitive identifier.

Otherwise things stop making sense. The JSON auth has a json-case-sensitive-usernames property but does not consume external identifiers, so the intent of that property has to be declaring the intent of what other extensions should do with identifiers from JSON auth. For that to be useful, we would need to meaningfully consume isCaseSensitive(), but most of the logic around consuming external identifiers on the database side depends solely on the database's *-case-sensitive-usernames property.

I'm starting to thing these changes have part of the right idea, and we need to change the rest to match.

[necouchman]

Hm ... maybe what I was thinking is actually backwards, and we should instead:

• Use the *-case-sensitive-usernames properties to dictate only whether users authenticating via that extension have case-sensitive usernames.
• Extensions that consume usernames from other extensions should honor the isCaseSensitive() value when comparing identities.

I'll go back through the previous changes and see what needs to be done to rework things using this logic.

• In the equality comparison for AbstractIdentifiable, we need some sane way to resolve what happens if you compare a case-sensitive identifier with a case-insensitive identifier.

I've pushed a commit that I think tackles this particular issue without too much hassle - let me know if that looks okay to you.

Otherwise things stop making sense. The JSON auth has a json-case-sensitive-usernames property but does not consume external identifiers, so the intent of that property has to be declaring the intent of what other extensions should do with identifiers from JSON auth. For that to be useful, we would need to meaningfully consume isCaseSensitive(), but most of the logic around consuming external identifiers on the database side depends solely on the database's *-case-sensitive-usernames property.

I'm starting to thing these changes have part of the right idea, and we need to change the rest to match.

Yeah, this makes sense to me. I'll review and see what changes need to be made to implement this logic.

[necouchman]

Otherwise things stop making sense. The JSON auth has a json-case-sensitive-usernames property but does not consume external identifiers, so the intent of that property has to be declaring the intent of what other extensions should do with identifiers from JSON auth. For that to be useful, we would need to meaningfully consume isCaseSensitive(), but most of the logic around consuming external identifiers on the database side depends solely on the database's *-case-sensitive-usernames property.
I'm starting to thing these changes have part of the right idea, and we need to change the rest to match.

Yeah, this makes sense to me. I'll review and see what changes need to be made to implement this logic.

So, if I'm thinking about this correctly, what this looks like a little more concretely is:

• The only reference to the various Environment calls for things like postgresql-case-sensitive-usernames would be in the JDBC User classes, to determine if the User identifiers should be treated as case-sensitive.
• In all of the various Service instances in the JDBC module (e.g. ModeledDirectoryObjectService), the call should not be to the Environment, but to the User object and whether it is case-(in)sensitive.

Does that sound right?

[mike-jumper]

Yes and no. That would be an alternative and consistent approach. Taking a mammoth step back to the beginning, I think we have two mutually-exclusive possible approaches:

1. Identifier case sensitivity is defined by the extension consuming the identifier.

   • This is what you're describing above and what I was describing initially.
   • This is invitingly simple, particularly for cases where there's only one thing consuming identifiers.
   • This feels a little odd, since it's the producer and not the consumer that determines the intent of the identifier, but it does mean there is no ambiguity in handling identifier comparisons.
   • This might make sense only in light of a single, global property for case sensitivity ... which is admittedly also invitingly simple.

2. Identifier case sensitivity is defined by the extension producing the identifier.

   • This is what I was describing most recently.
   • This is more complex, but allows the intent of an identifier to be communicated.
   • This feels cleaner, but introduces ambiguity around how case-sensitive and case-insensitive identifiers could be compared. Then again, encountering that explicit ambiguity might be an opportunity to log a warning that makes identifier case sensitivity mismatches easier to notice and remedy.

Migrating to the former would mean getting rid of isCaseSensitive(), instead relying purely on configuration properties determining how usernames should be handled, and reconsidering whether there should even be any extension-specific case sensitivity properties (since having intentionally mixed case sensitivity would be bad practice).

Migrating to the latter would mean keeping isCaseSensitive() and ensuring the documentation for the properties notes that they only control the case sensitivity of the usernames of users authenticated by and/or stored by that extension.

I find myself flipping back and forth between these without a clear answer as to which is truly best. I suspect that's probably also why the current approach is a sort of mix of these - they're both reasonable approaches.

[mike-jumper]

Then again ... it's hard to argue against the sheer simplicity and safety of losing isCaseSensitive() and just having case-sensitive-usernames.

[necouchman]

@mike-jumper @jmuehlner : I've taken a stab at an approach that should make the source for case-sensitivity more consistent across different login modules - essentially, what I've tried to do, is make sure that, whenever and wherever possible, particularly within the JDBC module, the source for the case sensitivity setting comes from the logged in user. The logged in user, in turn will pull from the environment for the current authentication module, which will either be that particular extension's configuration property or the global configuration property.

I just finished this up and have verified that it compiles - tomorrow I'll run it through some testing to make sure that the various use-cases that others identified that were broken are still functional.

If you spot any flaws in my logic, please give a shout. And, if we need to close it out and just go to a monolithic enable/disable option and scrap the whole per-extension setting, I can give that a run, too.

😩

[mike-jumper]

I think we do need to switch over to the monolithic enable/disable option (and +1 on the 😩).

As I review, I'm running into things like handling of usernames within history records, and there doesn't seem to be any way we can handle differences in case sensitivity sanely unless there is only the platform-wide option that states how all usernames are to be interpreted.

For example, let's say there are two authentication systems installed:

1. LDAP (configured to be case-insensitive)
2. PostgreSQL (configured to be case-sensitive)

If a user SOMEONE logs in with LDAP and accesses a connection, and then a user someone logs into PostgreSQL and accesses another connection, are they the same user? If a group in PostgreSQL includes the PostgreSQL user someone, does that include the LDAP user SOMEONE?

With a platform-wide option, things are completely unambiguous and everything is in agreement. With extension-specific options, things make sense as long as everything is manually configured to be in agreement, but stop making sense once there is any disagreement between extensions.

[necouchman]

I think we do need to switch over to the monolithic enable/disable option (and +1 on the 😩).

As I review, I'm running into things like handling of usernames within history records, and there doesn't seem to be any way we can handle differences in case sensitivity sanely unless there is only the platform-wide option that states how all usernames are to be interpreted.

For example, let's say there are two authentication systems installed:

1. LDAP (configured to be case-insensitive)
2. PostgreSQL (configured to be case-sensitive)

If a user SOMEONE logs in with LDAP and accesses a connection, and then a user someone logs into PostgreSQL and accesses another connection, are they the same user? If a group in PostgreSQL includes the PostgreSQL user someone, does that include the LDAP user SOMEONE?

With a platform-wide option, things are completely unambiguous and everything is in agreement. With extension-specific options, things make sense as long as everything is manually configured to be in agreement, but stop making sense once there is any disagreement between extensions.

Okay, I'll switch it up and go that route. It pains me a little bit to take away the flexibility of being able to choose on a per-extension basis, but the example you give above highlights the challenges with even making that available and the confusion that might arise were someone to try to debug that sort of situation.

The other thing that has continued to nag at the back of my mind is that I think we (ultimately) need to make the same option available for group names. This effort has focused almost exclusively on usernames, but I cannot help but think that the expectation of making usernames case-insensitive is going to easily make someone think that group names also ought to be case-insensitive. Thoughts on expanding the scope from just User objects to include UserGroup objects in this?

[mike-jumper]

... Thoughts on expanding the scope from just User objects to include UserGroup objects in this?

I agree. I'm sure this will be necessary and it makes sense to include this as part of the general configurable case sensitivity effort.

[necouchman]

... Thoughts on expanding the scope from just User objects to include UserGroup objects in this?

I agree. I'm sure this will be necessary and it makes sense to include this as part of the general configurable case sensitivity effort.

Okay - I think I've completed the re-work of the user code to only pull from the global configuration value, and removed all of the extension-based options. I have left the bits in place, mainly in the JDBC module, that pull from the user.isCaseSensitive() method, rather than pulling the environment directly - this tends to be a bit easier than injecting the environment everywhere it might be required, and also keeps the concern of whether or not a User identifier is case sensitive as part of the User object.

I'll see if I can knock the UserGroup portions out real quick - shouldn't be too terribly difficult.

And then...testing...

[necouchman]

I'll see if I can knock the UserGroup portions out real quick - shouldn't be too terribly difficult.

And then...testing...

Working through the UserGroup stuff this weekend. It's a little more involved that I anticipated...

[necouchman]

I'll see if I can knock the UserGroup portions out real quick - shouldn't be too terribly difficult.
And then...testing...

Working through the UserGroup stuff this weekend. It's a little more involved that I anticipated...

Almost there. Wrapping things up today. 🤞

[mike-jumper]

I think we should probably remove the isCaseSensitive() function, at least from guacamole-ext, so that it does not potentially result in conflicting information between an implementation's isCaseSensitive() vs. the global configuration option.

If it makes things easier to plumb within the JDBC auth, I don't see any issue with having a isCaseSensitive() function that's strictly internal to JDBC.

[necouchman]

Okay - I'm almost done with the changes - hoping to at least get the draft out tonight ahead of doing some testing, and you can see what I've done, there. I left isCaseSensitive() in, for now, because it helps with the comparisons between identifiers. But you can see what you think after I've pushed the remaining changes...

[mike-jumper]

Awesome - sounds reasonable to me.

[necouchman]

I'll see if I can knock the UserGroup portions out real quick - shouldn't be too terribly difficult.
And then...testing...

Working through the UserGroup stuff this weekend. It's a little more involved that I anticipated...

Almost there. Wrapping things up today. 🤞

Okay...here are the changes. I am 💯 certain that I have made multiple mistakes. I have done 0️⃣ testing after finishing these changes - my eyes are crossing from SO. MUCH. XML - while it compiles right now, I very much doubt that it will actually work out of the gate. There are also several style issues that need fixing. But, any overall concerns or comments about the approach, let me know.

[mike-jumper]

my eyes are crossing from SO. MUCH. XML

Oof, I don't blame them, but thanks for doing this. I think we're on the right path.

[necouchman]

my eyes are crossing from SO. MUCH. XML

Oof, I don't blame them, but thanks for doing this. I think we're on the right path.

Sounds good. I'm working through testing, now, and have already corrected a couple of issues, which I'll push, here, shortly.

[necouchman]

I've removed this from draft state, as I've done several rounds of testing, fixed several items, and am cautiously optimistic that it is working as intended and that I've corrected the typos. If others are inclined to test, that would be welcome - so far I've only tested with PostgreSQL, as that is my DB of choice. I'll try to spin up MySQL and SQL Server at some point, here, soon, and test with those.

[mike-jumper]

I rather like these changes, particularly the clean approach of using CaseSensitivity to represent both usernames and group names.

I've found only a few minor issues, with a couple probably worth a quick grep to look for other occurrences:

• In several locations, "case sensitivity" is written as "case-sensitivity". I believe "case sensitivity" is the correct way, with the hyphen used only for things like "case-sensitive" and "case-insensitive".
• The exception caught when attempting to read the case sensitivity value from guacamole.properties is variously logged without the {} placeholder for e.getMessage() or with just e (full stack trace) instead of e.getMessage().

Everything else looks great.