-
Notifications
You must be signed in to change notification settings - Fork 681
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
GUACAMOLE-103: Implement SAML Authentication Extension #254
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's my initial self-review...appreciate any comments...
...le-auth-saml/src/main/java/org/apache/guacamole/auth/saml/AuthenticationProviderService.java
Show resolved
Hide resolved
...th-saml/src/main/java/org/apache/guacamole/auth/saml/SAMLAuthenticationProviderResource.java
Outdated
Show resolved
Hide resolved
...camole-auth-saml/src/main/java/org/apache/guacamole/auth/saml/conf/ConfigurationService.java
Outdated
Show resolved
Hide resolved
...camole-auth-saml/src/main/java/org/apache/guacamole/auth/saml/conf/ConfigurationService.java
Outdated
Show resolved
Hide resolved
...camole-auth-saml/src/main/java/org/apache/guacamole/auth/saml/conf/ConfigurationService.java
Outdated
Show resolved
Hide resolved
guacamole-ext/src/main/java/org/apache/guacamole/properties/UrlGuacamoleProperty.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some licensing changes.
extensions/guacamole-auth-saml/src/licenses/bundled/saml-client-1.2.0/LICENSE
Outdated
Show resolved
Hide resolved
0f96e13
to
3304d89
Compare
3304d89
to
6330e8f
Compare
0de47a0
to
4245704
Compare
@necouchman how i can get Guacamole working with CAS for SAML authentication |
@saydulk http://guacamole.apache.org/support/ |
4245704
to
90bf6e3
Compare
90bf6e3
to
ae9a66a
Compare
1ef52fe
to
bf2f6cd
Compare
Hello, out of interest: what are the latest plans to integrate SAML into a official release of guacamole? Thx & Regards, Michael |
@Saphirim As you can see, this is still in the works - it needs to be reviewed and probably has some work to do on it before it's ready. Should be pretty close, but we've also got some high-priority items we're trying to work for the next release, so not certain it will make it. SSO is already available via other extensions - CAS, OpenID, and Header modules - so you have some options, but if you require SAML you'll have to wait for this 😄. |
I would like to donate some money to speed up the SAML SSO and SLO support for guacamole. I am waiting to upgrade my pam auth with SLO and SSO. I am using an ipsilon SAML IdP and I would like to test the new extension when ready. If you need a test infrastructure you can contact me and I will share some accounts and an IdP and SP setup. |
@tuxcrafter: You're welcome to go ahead and try the extension, now. It does not support SLO, yet, but should work with version 1.0.0 for logging in. |
@necouchman We compiled and install the saml extension but I can not figure out how to configure the saml settings. I probably need to configure the following settings somewhere: idp-entityId (metadata) And have the extension generate the correct metadata file for guacamole saml, to be used in the idp. Then it would be good to have a mapping options for the username and groups. |
@necouchman Could we get a sample configuration file for the saml extension? It would enable us to start hacking on our saml/guacamole setup. |
@LeBrad @tuxcrafter
Note that if you have an XML metadata file from your SAML IDP, you should not need the rest of those details - all of those configuration items should be in the XML file. Anyway, test away, feel free to let me know if you run into any issues with it. I do need to rebase it against the latest |
Thanks for the properties.
Here's my config at the moment:
|
Here are the answers...I think...
|
Hi, I pulled this, and was attempting to test the SAML auth extension. everything seems to be building / deploying fine. but the issue I have is with setting the configuration. I specified the idp xml file : saml-idp-metadata: /etc/guacamole/sample-saml.xml I'm running with tomcat 7, and regardless of how I specify the saml-idp-metadata, or where I put the .xml file, i always get the same error : ERROR c.o.saml2.settings.IdPMetadataParser - XML file'/etc/guacamole/sample-saml.xml' cannot be loaded.XML file '/etc/guacamole/sample-saml.xml' not found in the classpath I've tried specifying it in relative / absolute paths I've tried putting it in /etc/guacamole, $TOMCAT_HOME, $TOMCAT_BASE, $TOMCAT_HOME/lib, $TOMCAT_BASE/lib, the resources folder in the source code to get into the .jar extension. I added /etc/guacamole to the catalina properties common.loader, and shared.loader if anyone has any ideas, I'd love to hear them. thanks |
Adding myself to the CC list. |
5dde4bf
to
b16535f
Compare
8ffbc64
to
925af02
Compare
Okay, quite a bit of refactoring done, here, and I've tested successfully against the CAS implementation of SAML. |
20ea6bc
to
52318a9
Compare
Well, that was fun. Should all be rebased on the current |
c513aac
to
2e92a9b
Compare
What happened that made this rebase particularly hairy? |
The changes to implement the common Also, this has been going on for 18 months or so, now, I think - maybe 2 years at this point - so there were lots of version updates and license changes along the way. Extracting the |
Yeah, pretty much. |
...le-auth-saml/src/main/java/org/apache/guacamole/auth/saml/AuthenticationProviderService.java
Outdated
Show resolved
Hide resolved
...le-auth-saml/src/main/java/org/apache/guacamole/auth/saml/AuthenticationProviderService.java
Outdated
Show resolved
Hide resolved
...th-saml/src/main/java/org/apache/guacamole/auth/saml/SAMLAuthenticationProviderResource.java
Outdated
Show resolved
Hide resolved
...nsions/guacamole-auth-saml/src/main/java/org/apache/guacamole/auth/saml/SAMLResponseMap.java
Outdated
Show resolved
Hide resolved
...camole-auth-saml/src/main/java/org/apache/guacamole/auth/saml/conf/ConfigurationService.java
Outdated
Show resolved
Hide resolved
...nsions/guacamole-auth-saml/src/main/java/org/apache/guacamole/auth/saml/SAMLResponseMap.java
Outdated
Show resolved
Hide resolved
...nsions/guacamole-auth-saml/src/main/java/org/apache/guacamole/auth/saml/SAMLResponseMap.java
Show resolved
Hide resolved
...camole-auth-saml/src/main/java/org/apache/guacamole/auth/saml/conf/ConfigurationService.java
Outdated
Show resolved
Hide resolved
...amole-auth-saml/src/main/java/org/apache/guacamole/auth/saml/user/SAMLAuthenticatedUser.java
Outdated
Show resolved
Hide resolved
2e92a9b
to
206d47b
Compare
Includes implementation of executor shutdown, and correctly removing items from the shared response map.
206d47b
to
806ec96
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I've re-reviewed things in their entirety. I find only two things which for sure should be fixed:
- Unsafe use of
remove()
while iterating aCollection
- A minor typo
and there are two things which may need to be fixed, depending on your view and technical reasoning behind the current approach:
- Prompting the user for a standard username/password when an error is encountered with SAML, rather than presenting an error screen.
- Generating parameter tokens without canonicalizing and prefixing the names.
...nsions/guacamole-auth-saml/src/main/java/org/apache/guacamole/auth/saml/SAMLResponseMap.java
Outdated
Show resolved
Hide resolved
...le-auth-saml/src/main/java/org/apache/guacamole/auth/saml/AuthenticationProviderService.java
Outdated
Show resolved
Hide resolved
...le-auth-saml/src/main/java/org/apache/guacamole/auth/saml/AuthenticationProviderService.java
Outdated
Show resolved
Hide resolved
...le-auth-saml/src/main/java/org/apache/guacamole/auth/saml/AuthenticationProviderService.java
Outdated
Show resolved
Hide resolved
Exception handling within the SAML extension has been updated such that exceptions generate a Guacamole Error rather than a username/password login. Tokens now are canonicalized with a standard prefix. Now using an Iterator to handle SAMLResponseMap cleanup.
One additional thing I'm seeing in the SAML client - when the IdP redirects back to the Guacamole Client and the login completes successfully, the Is there something I should do (similar to the OpenID extension, maybe?) to remove this after a successful login? |
I don't think the OpenID extension removes this, either, but it handles the presence of an invalid token identically to the total absence of a token. You could do the same - log at the info or debug level that an invalid hash was received and then move forward requesting a new SAML response? |
Ah, okay, yes - I've tweaked it so that it should behave that way - I'll verify when I get a moment to recompile and test. |
Okay, redirect appears to be working okay, now. One more issue I need to fix, though - when you try to specify the
I guess whatever method I've used to load the file doesn't accept absolutely paths, or requires a URI or something like that. |
Okay, I switched the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alrighty - odd that parseFileXML()
is restricted to the classpath and does not touch the filesystem, but leveraging URLs and possible retrieval of remote XML seems a fine solution.
Given that the property now accepts a URL, I think it should be given a -url
suffix to stay consistent with the other URL properties. Other than that, if your testing shows things are now working correctly, I'm good with these changes.
...camole-auth-saml/src/main/java/org/apache/guacamole/auth/saml/conf/ConfigurationService.java
Outdated
Show resolved
Hide resolved
fbf176c
to
a493db3
Compare
a493db3
to
8acb3cb
Compare
I've tested successfully with the CAS SAML provider, both with a metadata file (using a file:// URI) and with manually specifying the parameter for accessing the server. If I can find another SAML provider or two to test with I will try to get that done, but I don't have ready access to any aside from CAS. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me.
I'll try to test verify against G Suite's SAML, as well, but I think things are very well confirmed at this point.
@necouchman Hello can you provide information how to provide connection to user logged in with saml authentication? |
@puneetshukla-siemens: This is a closed pull request, and is not intended to be a support forum. Please subscribe to the mailing list and ask your question, there: |
I tried with OKTA pulling the metadata from a file. Problem I get is an endless loop between OKTA and the url callback. Somehow after OKTA authenticates successfully, Guacamole is not capturing that. |
@JPedroGon: Please post your questions to the mailing list. This is a closed/merged pull request and is not designed to be a support forum. |
Initial cut at a SAML authentication extension. Plenty of room for improvement, I'm sure - I welcome all comments/suggestions/changes. I'll kick off my own review, here, shortly.