Re: Setting up HTTP header authentication

[Dmitry Katsubo <dm...@mail.ru.INVALID>]

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.

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.

On 2019-03-22 21:42, Nick Couchman wrote:
>
>>     Yes, we removed the NoAuth module without replacing it.  The project determined that it was not worth continuing to keep it in the code, as the value was limited and the end-goal of the module
>>     - transparently authenticating users into Guacamole - was possible by several other more secure means (SSO and parameter tokens, in particular).  It's also true that the header module is very
>>     simple - it accepts that a user has been authenticated up-stream and relies on other modules to provide configurations.  This comes with a security caveat of its own - if you use the header
>>     module it *must* be behind a reasonably secure front-end proxy that won't allow someone to spoof the header that is then accepted by the authentication module.  There are warnings about this in
>>     the manual.
>     I agree. On the other hand, even if we make FileAuthenticationProvider work properly, JDBCAuthenticationProviderModule will still not work, as it requires username/password for authentication
>     against the database. So if there is a need to stack JDBC/LDAP on the top of header authentication, one needs to agree how to enable that.
>
>
> This is not accurate - I've used the Header module with the JDBC module repeatedly, and it works fine, even without a password being provided.  The JDBC module will recognize users authenticated by
> any other module - LDAP, Header, CAS, OpenID, RADIUS - regardless of whether the module sets a password on the Credential object.  The File handler does not currently behave that way.  The LDAP
> module, when used to store connections, also relies on both the username and password to be available because it binds to the LDAP tree with the provided username and password.  The JDBC module uses
> a fixed username and password to access the database, and accepts authentication from other modules matching via username only.
>
On 2019-03-26 00:30, Nick Couchman wrote:
> The site you referenced is for the Apache Directory project, not the Guacamole project.  Our main page is here:
>
> http://guacamole.apache.org
>
> And the contribution guidelines are here:
>
> http://guacamole.apache.org/open-source/
>
> With specific style guidelines noted here:
>
> http://guacamole.apache.org/guac-style/

-- 
With best regards,
Dmitry

Re: Setting up HTTP header authentication

[Nick Couchman <vn...@apache.org>]

On Tue, May 24, 2022 at 6:31 AM Dmitry Katsubo <dm...@mail.ru.invalid>
wrote:

> 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.
>

You should use gaucamole-ext as the dependency for the module, plus
whatever individual dependencies you need. If you have to add other
dependencies, that's fine - I think if you were to examine the pom.xml
files across the project you'd see that there are some dependencies that
are duplicated. Is it the maximumly-efficient way to go? Maybe not;
however, it allows the extensions to be pluggable/interchangeable, and for
the framework of Guacamole to be re-used for other things. It's a
trade-off, and sometimes absolutely efficiency is sacrificed for
compatibility, ease of reuse, etc.

> 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.
>
You're certainly welcome to modify the code you have to fit how you want to
do it - I would say _that_ is at least one of the things that Open Source
is about. As a project, our goals may be different from your individual
use-case, and that's okay - you have the source code, you can modify it as
you see fit. However, the changes that you've suggested are not ones that
we're willing to incorporate into the main code base for the project as
they stand today, for the reasons already mentioned.

-Nick

>

Re: Setting up HTTP header authentication

[Dmitry Katsubo <dm...@mail.ru.INVALID>]

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 <dm...@mail.ru.invalid> 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

Re: Setting up HTTP header authentication

[Nick Couchman <vn...@apache.org>]

On Thu, May 19, 2022 at 10:52 AM Dmitry Katsubo <dm...@mail.ru.invalid>
wrote:

> On 2022-05-19 01:44, Michael Jumper wrote:
>
> On Mon, May 16, 2022 at 12:23 PM Dmitry Katsubo <dm...@mail.ru.invalid>
> <dm...@mail.ru.invalid> 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

>

Re: Setting up HTTP header authentication

[Dmitry Katsubo <dm...@mail.ru.INVALID>]

On 2022-05-19 01:44, Michael Jumper wrote:
> On Mon, May 16, 2022 at 12:23 PM Dmitry Katsubo <dm...@mail.ru.invalid> 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.
> 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)?
> 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.
From another side if the changes I suggest break some other flow that you have in mind, like proper data flow in conjunction with some other extension – please let me know how can I reproduce the
issue, so that I can improve the code changes I suggested.

Many thanks!

-- 
With best regards,
Dmitry

Re: Setting up HTTP header authentication

[Michael Jumper <mj...@apache.org>]

On Mon, May 16, 2022 at 12:23 PM Dmitry Katsubo <dm...@mail.ru.invalid>
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.


> 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.

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).

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.).

- Mike