You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@guacamole.apache.org by dotmaster <gi...@git.apache.org> on 2017/07/07 12:29:56 UTC
[GitHub] incubator-guacamole-client pull request #174: GUACAMOLE-341: Make SSO Authen...
GitHub user dotmaster opened a pull request:
https://github.com/apache/incubator-guacamole-client/pull/174
GUACAMOLE-341: Make SSO Authentication work and pass through username…
… to RDP session
To make tokenfilter work with auth-header and noauth module. (needed e.g. for Kerberos authentication through an Apache/Nginx Reverse Proxy, that passes REMOTE_USER header), username must be set in the credentials object, because it is added to the Tokenfilter only if username is not null in the credentials object.
basically make $
{GUAC_USERNAME}
be replaced with the credentials passed though the REMOTE_USER variable.
See also:
https://github.com/glyptodon/guacamole-client/blob/b26a664d66fd14a22eb7300d29aa20390cf408ec/guacamole-ext/src/main/java/org/apache/guacamole/token/StandardTokens.java#L118
https://github.com/glyptodon/guacamole-client/blob/0.9.12-incubating/guacamole-ext/src/main/java/org/apache/guacamole/net/auth/simple/SimpleAuthenticationProvider.java#L170
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/dotmaster/incubator-guacamole-client patch-1
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-guacamole-client/pull/174.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #174
----
commit 7c408534ce4bfd3093ae2d9cca2de56c0cb826e2
Author: dotmaster <is...@gmail.com>
Date: 2017-07-07T12:29:34Z
GUACAMOLE-341: Make SSO Authentication work and pass through username to RDP session
To make tokenfilter work with auth-header and noauth module. (needed e.g. for Kerberos authentication through an Apache/Nginx Reverse Proxy, that passes REMOTE_USER header), username must be set in the credentials object, because it is added to the Tokenfilter only if username is not null in the credentials object.
basically make $
{GUAC_USERNAME}
be replaced with the credentials passed though the REMOTE_USER variable.
See also:
https://github.com/glyptodon/guacamole-client/blob/b26a664d66fd14a22eb7300d29aa20390cf408ec/guacamole-ext/src/main/java/org/apache/guacamole/token/StandardTokens.java#L118
https://github.com/glyptodon/guacamole-client/blob/0.9.12-incubating/guacamole-ext/src/main/java/org/apache/guacamole/net/auth/simple/SimpleAuthenticationProvider.java#L170
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-guacamole-client pull request #174: GUACAMOLE-341: Make SSO Authen...
Posted by dotmaster <gi...@git.apache.org>.
Github user dotmaster commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/174#discussion_r126350683
--- Diff: extensions/guacamole-auth-header/src/main/java/org/apache/guacamole/auth/header/AuthenticationProviderService.java ---
@@ -71,6 +71,9 @@ public AuthenticatedUser authenticateUser(Credentials credentials)
// Get the username from the header configured in guacamole.properties
String username = request.getHeader(confService.getHttpAuthHeader());
+
+ //write username to the credentials object to make tokenfilter work
--- End diff --
@mike-jumper changed the comment style to the coding style guide.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-guacamole-client pull request #174: GUACAMOLE-341: Make SSO Authen...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/174#discussion_r126514786
--- Diff: extensions/guacamole-auth-header/src/main/java/org/apache/guacamole/auth/header/AuthenticationProviderService.java ---
@@ -73,6 +73,8 @@ public AuthenticatedUser authenticateUser(Credentials credentials)
String username = request.getHeader(confService.getHttpAuthHeader());
if (username != null) {
+ // Write username to the credentials object to make tokenfilter work
--- End diff --
"to make tokenfilter work" sounds like this is a stopgap measure / hack just to get things working for a very specific case, which isn't really the kind of change that should ever be made to any part of the Guacamole codebase.
If `TokenFilter` is not working as expected because this value isn't populated, and the fact that it isn't populated is a bug, then that's a legitimate reason to make this change, and there's no need to call out the specific things that break because it's absent. If it's necessary, it's necessary.
If this truly is a hack, however, it might be worth instead looking into ways that `TokenFilter` could be modified to not depend in the username in the `Credentials` object. Grabbing the identifier of the `AuthenticatedUser` might be an alternative, though I haven't thought through whether that would go against the defined semantics of the `GUAC_USERNAME` token.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-guacamole-client pull request #174: GUACAMOLE-341: Make SSO Authen...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/incubator-guacamole-client/pull/174
---
[GitHub] incubator-guacamole-client pull request #174: GUACAMOLE-341: Make SSO Authen...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/174#discussion_r126162449
--- Diff: extensions/guacamole-auth-header/src/main/java/org/apache/guacamole/auth/header/AuthenticationProviderService.java ---
@@ -71,6 +71,9 @@ public AuthenticatedUser authenticateUser(Credentials credentials)
// Get the username from the header configured in guacamole.properties
String username = request.getHeader(confService.getHttpAuthHeader());
+
+ //write username to the credentials object to make tokenfilter work
--- End diff --
Please follow the existing code style. Compare:
> //write username to the credentials object to make tokenfilter work
vs. other comments in incubator-guacamole-client:
> // Get the username from the header configured in guacamole.properties
Relative to the established style, this comment is missing the space following `//`, and should be in sentence case (first letter capitalized).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-guacamole-client pull request #174: GUACAMOLE-341: Make SSO Authen...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/174#discussion_r132784755
--- Diff: extensions/guacamole-auth-header/src/main/java/org/apache/guacamole/auth/header/AuthenticationProviderService.java ---
@@ -73,6 +73,8 @@ public AuthenticatedUser authenticateUser(Credentials credentials)
String username = request.getHeader(confService.getHttpAuthHeader());
if (username != null) {
+ // Write username to the credentials object to make tokenfilter work
--- End diff --
@mike-jumper Any thoughts here on whether it's preferable to set the username inside this module, or modify code elsewhere to use the AuthenticatedUser identifier? Going the AuthenticatedUser route looks like it would require one of the following approaches:
- Another method in StandardTokens to be able to pass in the username token, specifically, with its own identifier, and then changes to the various places that StandardTokens is used to add both the credentials and then, alternatively the AuthenticatedUser code.
- Checks around the existing StandardTokens uses that make sure the Credentials object has a valid username, and then code to create a new object or modify the existing one around there using the AuthenticatedUser object.
It seems to me that setting it up inside the authentication module is the right way to go - it results in the fewest places that have to be reworked, and makes it available across the various places where those Credentials objects are used. This module (auth-header) needs the fix, as will the CAS module. I think those are the only two at the moment - any additional SSO-type modules would also have to keep it in mind (SAML, OAuth).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---