On Thu, May 19, 2022 at 10:52 AM Dmitry Katsubo <[email protected]> wrote:
> On 2022-05-19 01:44, Michael Jumper wrote: > > On Mon, May 16, 2022 at 12:23 PM Dmitry Katsubo <[email protected]> > <[email protected]> wrote: > >> Dear Guacamole users, >> Dear Nick, >> >> Sorry I decided to resurrect the 4-years old challenge. I have rebased my >> changes on the latest codebase. Not so many changes are required to allow >> the user authenticated via auth-header extension to be provided >> authentication information / connection settings from user-mapping.xml. >> Without the changes the settings are not picked up from user-mapping.xml. >> > > Is there a specific reason that you cannot use the database? It's intended > for what you describe, intended for production use, and will work with > header auth. > > I think that database is overkill for systems that have a couple of users > (e.g. remote admins). Files are easier to maintain and backup, as all > Guacamole configuration is basically located in one place. Also imagine the > situation when database is down and could be fixed with help of Guacamole > unless it is running on the top of that very database. > > Please check my commit b0aa658 >> <https://github.com/dmak/guacamole-client/commit/b0aa658043689b8ff37d18db49a75ac443b4cc12>. >> If that is OK, then I would provide few unit tests for it. Otherwise let me >> know what is missing, preferably in terms so that I can implement a test. >> > > Looking at your commit, I see that one of the primary changes here is > changing the prototype and visibility of the getAuthorizedConfigurations() > function. This will break API and ABI compatibility, and I do not think we > should do this. > > You mean that there are classes that extend SimpleAuthenticationProvider > which are outside Guacamole git? Could be of course, however their > adaptation will be trivial. > Yes, but the point is that Guacamole is designed to provide not just a framework for itself, but one that people can build upon. With that in mind, API/ABI changes need to be very carefully considered, and also need to be made to be as backward-compatible as possible. In the past we've done things like deprecate methods or classes, but they remain available in the deprecated state for many releases before they are finally removed completely. The changes need to be made in such a way that they don't automatically break things for people who may be using/extending these classes, and that they have the option of continuing to use them in the way they are written while they change their code to the new way, but are warned that support for it may be removed/changed at some point in the future. > For the built-in support for user-mapping.xml to be able to accept the > authentication results of other installed extensions, it will need to be > modified to use the less-simple API and implement AuthenticationProvider > and UserContext (rather than use SimpleAuthenticationProvider). > > I think that should be possible. AuthenticationProvider is already > implemented, probably not the proper way (if so, what is missing?). As for > UserContext I am not sure: none of the providers I've checked implement > this interface. Maybe you mean that SimpleUserContext should implement > that interface in a proper way (again what exactly is missing)? > It is definitely possible, just needs to be done. I would also say it's worth considering leaving the existing user-mapping.xml authentication mechanism as-is and just implementing a different file-based one. It could be XML, or YAML, or JSON (or provide methods for reading any/all of those file types), and would be another extension in the "extensions/" folder. > With user-mapping.xml really being intended for testing only, and with > these changes aimed at allowing user-mapping.xml to be used in a more > complex configuration aimed at production use, I think these changes really > would need to be coupled with a move to a user-mapping variant that *is* > intended > for production (proper salted hashes for passwords instead of > intentionally-simplified-for-testing hashes, the ability to define a > user/connection association that requires auth from some other extension > and otherwise has no password, etc.). > > I think there are two things here mixed. The password which is used to > authenticate the user against Guacamole is of course salted hashed and > stored in guacamole_user SQL table. However in the setup I have the user > is already authenticated by the front Web server, hence the password is > null. There is nothing to salt or hash. On the other side the password > stored in guacamole_connection_attribute table I believe is saved in > plaintext, right? In this respect I don't see what else can be improved in > user-mapping.xml which is basically another representation of the data in > SQL database. > What you're asking for is a way to simply store connections in a file and delegate the authentication elsewhere - the point is that the changes you've made to the built-in test authentication mechanism are not necessarily the best way to go about that, because you have to consider how other people will continue to use those mechanisms - will it break other things that rely on it, or will it encourage people to use those mechanisms in an insecure manner? This is another reason why I think implementing a separate file-based extension, rather than making tweaks to the built-in default one, is probably a better way to go. -Nick >
