Thanks to everyone for his comments. On 2022-05-19 17:20, Nick Couchman wrote: > On Thu, May 19, 2022 at 10:52 AM Dmitry Katsubo <[email protected]> wrote: > > > 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. OK, I see. > >> 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. I have analysed the code and I see that most of the classes (e.g. those needed to parse XML) are located in guacamole module, which probably cannot be used as dependency for an extension. So it looks that about 5 classes are to be copied as is to "new extension" module. Does not smell good in terms of code reusability. > >> 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. > I am OK that the changes I suggest do not fit the common perception about how API should be organized. For me it is more logical to keep 10 lines of code patch that perfectly fits my needs rather than re-invent the extension that will be a copy-paste of existing code with no added value. At the end of the day that what OpenSource is about.
-- With best regards, Dmitry
