mike-jumper commented on a change in pull request #254: URL: https://github.com/apache/guacamole-client/pull/254#discussion_r444748256
########## File path: extensions/guacamole-auth-saml/src/main/java/org/apache/guacamole/auth/saml/AuthenticationProviderService.java ########## @@ -0,0 +1,289 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.guacamole.auth.saml; + +import com.google.inject.Inject; +import com.google.inject.Provider; +import com.onelogin.saml2.authn.AuthnRequest; +import com.onelogin.saml2.authn.SamlResponse; +import com.onelogin.saml2.exception.SettingsException; +import com.onelogin.saml2.exception.ValidationError; +import com.onelogin.saml2.settings.Saml2Settings; +import com.onelogin.saml2.util.Util; +import java.io.IOException; +import java.net.URI; +import java.net.URISyntaxException; +import java.util.Arrays; +import java.util.Collections; +import java.util.HashMap; +import java.util.HashSet; +import java.util.List; +import java.util.Map; +import java.util.Map.Entry; +import java.util.Set; +import javax.servlet.http.HttpServletRequest; +import javax.xml.parsers.ParserConfigurationException; +import javax.xml.xpath.XPathExpressionException; +import org.apache.guacamole.auth.saml.conf.ConfigurationService; +import org.apache.guacamole.auth.saml.user.SAMLAuthenticatedUser; +import org.apache.guacamole.GuacamoleException; +import org.apache.guacamole.form.Field; +import org.apache.guacamole.form.RedirectField; +import org.apache.guacamole.language.TranslatableMessage; +import org.apache.guacamole.net.auth.AuthenticatedUser; +import org.apache.guacamole.net.auth.Credentials; +import org.apache.guacamole.net.auth.credentials.CredentialsInfo; +import org.apache.guacamole.net.auth.credentials.GuacamoleInvalidCredentialsException; +import org.apache.guacamole.net.auth.credentials.GuacamoleInsufficientCredentialsException; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; +import org.xml.sax.SAXException; + +/** + * Class that provides services for use by the SAMLAuthenticationProvider class. + */ +public class AuthenticationProviderService { + + /** + * Logger for this class. + */ + private final Logger logger = LoggerFactory.getLogger(AuthenticationProviderService.class); + + /** + * Service for retrieving SAML configuration information. + */ + @Inject + private ConfigurationService confService; + + /** + * Provider for AuthenticatedUser objects. + */ + @Inject + private Provider<SAMLAuthenticatedUser> authenticatedUserProvider; + + /** + * The map used to track active SAML responses. + */ + @Inject + private SAMLResponseMap samlResponseMap; + + /** + * Returns an AuthenticatedUser representing the user authenticated by the + * given credentials. + * + * @param credentials + * The credentials to use for authentication. + * + * @return + * An AuthenticatedUser representing the user authenticated by the + * given credentials. + * + * @throws GuacamoleException + * If an error occurs while authenticating the user, or if access is + * denied. + */ + public AuthenticatedUser authenticateUser(Credentials credentials) + throws GuacamoleException { + + HttpServletRequest request = credentials.getRequest(); + + // Initialize and configure SAML client. + Saml2Settings samlSettings = confService.getSamlSettings(); + + if (request != null) { + + // Look for the SAML Response parameter. + String responseHash = request.getParameter("responseHash"); + + if (responseHash != null) { + + try { + + // Generate the response object + if (!samlResponseMap.hasSamlResponse(responseHash)) { + logger.warn("SAML response was not found."); + logger.debug("SAML response hash {} not fonud in response map.", responseHash); + throw new GuacamoleInvalidCredentialsException("Provided response was not found.", + CredentialsInfo.USERNAME_PASSWORD); Review comment: Can you clarify the reasoning behind the error handling? I notice that all errors during authentication are handled by issuing a request for a standard username/password via `GuacamoleInvalidCredentialsException`, rather than reporting a server error with `GuacamoleServerException`. My view would be that issuing a `GuacamoleServerException` would be the correct behavior (resulting in a generic error screen) rather than `GuacamoleInvalidCredentialsException` (resulting in a login screen that prompts for credentials this extension doesn't accept), but I'm OK with the current approach if there is a technical reason that it needs to be done this way. I do see: ``` // Errors are logged and result in a normal username/password login box. ``` but it's unclear to me why that should be done. ########## File path: extensions/guacamole-auth-saml/src/main/java/org/apache/guacamole/auth/saml/SAMLResponseMap.java ########## @@ -0,0 +1,133 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.guacamole.auth.saml; + +import com.google.inject.Singleton; +import com.onelogin.saml2.authn.SamlResponse; +import com.onelogin.saml2.exception.ValidationError; +import java.util.Collection; +import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.ConcurrentMap; +import java.util.concurrent.Executors; +import java.util.concurrent.ScheduledExecutorService; +import java.util.concurrent.TimeUnit; + +/** + * A class that handles mapping of hashes to SAMLResponse objects. + */ +@Singleton +public class SAMLResponseMap { + + /** + * The internal data structure that holds a map of SHA-256 hashes to + * SAML responses. + */ + private final ConcurrentMap<String, SamlResponse> samlResponseMap = + new ConcurrentHashMap<>(); + + /** + * Executor service which runs the periodic cleanup task + */ + private final ScheduledExecutorService executor = + Executors.newScheduledThreadPool(1); + + /** + * Create a new instance of this response map and kick off the executor + * that schedules the response cleanup task to run every five minutes. + */ + public SAMLResponseMap() { + // Cleanup unclaimed responses every five minutes + executor.scheduleAtFixedRate(new SAMLResponseCleanupTask(), 5, 5, TimeUnit.MINUTES); + } + + /** + * Retrieve the SamlResponse from the map that is represented by the + * provided hash, or null if no such object exists. + * + * @param hash + * The SHA-256 hash of the SamlResponse. + * + * @return + * The SamlResponse object matching the hash provided. + */ + protected SamlResponse getSamlResponse(String hash) { + return samlResponseMap.remove(hash); + } + + /** + * Place the provided mapping of hash to SamlResponse into the map. + * + * @param hash + * The hash that will be the lookup key for this SamlResponse. + * + * @param samlResponse + * The SamlResponse object. + */ + protected void putSamlResponse(String hash, SamlResponse samlResponse) { + samlResponseMap.put(hash, samlResponse); + } + + /** + * Return true if the provided hash key exists in the map, otherwise false. + * + * @param hash + * The hash key to look for in the map. + * + * @return + * true if the provided hash is present, otherwise false. + */ + protected boolean hasSamlResponse(String hash) { + return samlResponseMap.containsKey(hash); + } + + /** + * Task which runs every five minutes and cleans up any expired SAML + * responses that haven't been claimed and removed from the map. + */ + private class SAMLResponseCleanupTask implements Runnable { + + @Override + public void run() { + + // Loop through responses in map and remove ones that are no longer valid. + Collection<SamlResponse> samlResponses = samlResponseMap.values(); + for (SamlResponse value : samlResponses) { + try { + value.validateTimestamps(); + } + catch (ValidationError e) { + samlResponses.remove(value); + } + } Review comment: Just as with `entrySet()`, it's not safe to call `remove()` on the collection while iterating that collection. The only safe ways to remove items while iterating are: * Use an `Iterator` and invoke `remove()` on the iterator (not on the collection being iterated). * Use the fancy `removeIf()` that we get with Java 8. ########## File path: extensions/guacamole-auth-saml/src/main/java/org/apache/guacamole/auth/saml/AuthenticationProviderService.java ########## @@ -0,0 +1,289 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.guacamole.auth.saml; + +import com.google.inject.Inject; +import com.google.inject.Provider; +import com.onelogin.saml2.authn.AuthnRequest; +import com.onelogin.saml2.authn.SamlResponse; +import com.onelogin.saml2.exception.SettingsException; +import com.onelogin.saml2.exception.ValidationError; +import com.onelogin.saml2.settings.Saml2Settings; +import com.onelogin.saml2.util.Util; +import java.io.IOException; +import java.net.URI; +import java.net.URISyntaxException; +import java.util.Arrays; +import java.util.Collections; +import java.util.HashMap; +import java.util.HashSet; +import java.util.List; +import java.util.Map; +import java.util.Map.Entry; +import java.util.Set; +import javax.servlet.http.HttpServletRequest; +import javax.xml.parsers.ParserConfigurationException; +import javax.xml.xpath.XPathExpressionException; +import org.apache.guacamole.auth.saml.conf.ConfigurationService; +import org.apache.guacamole.auth.saml.user.SAMLAuthenticatedUser; +import org.apache.guacamole.GuacamoleException; +import org.apache.guacamole.form.Field; +import org.apache.guacamole.form.RedirectField; +import org.apache.guacamole.language.TranslatableMessage; +import org.apache.guacamole.net.auth.AuthenticatedUser; +import org.apache.guacamole.net.auth.Credentials; +import org.apache.guacamole.net.auth.credentials.CredentialsInfo; +import org.apache.guacamole.net.auth.credentials.GuacamoleInvalidCredentialsException; +import org.apache.guacamole.net.auth.credentials.GuacamoleInsufficientCredentialsException; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; +import org.xml.sax.SAXException; + +/** + * Class that provides services for use by the SAMLAuthenticationProvider class. + */ +public class AuthenticationProviderService { + + /** + * Logger for this class. + */ + private final Logger logger = LoggerFactory.getLogger(AuthenticationProviderService.class); + + /** + * Service for retrieving SAML configuration information. + */ + @Inject + private ConfigurationService confService; + + /** + * Provider for AuthenticatedUser objects. + */ + @Inject + private Provider<SAMLAuthenticatedUser> authenticatedUserProvider; + + /** + * The map used to track active SAML responses. + */ + @Inject + private SAMLResponseMap samlResponseMap; + + /** + * Returns an AuthenticatedUser representing the user authenticated by the + * given credentials. + * + * @param credentials + * The credentials to use for authentication. + * + * @return + * An AuthenticatedUser representing the user authenticated by the + * given credentials. + * + * @throws GuacamoleException + * If an error occurs while authenticating the user, or if access is + * denied. + */ + public AuthenticatedUser authenticateUser(Credentials credentials) + throws GuacamoleException { + + HttpServletRequest request = credentials.getRequest(); + + // Initialize and configure SAML client. + Saml2Settings samlSettings = confService.getSamlSettings(); + + if (request != null) { + + // Look for the SAML Response parameter. + String responseHash = request.getParameter("responseHash"); + + if (responseHash != null) { + + try { + + // Generate the response object + if (!samlResponseMap.hasSamlResponse(responseHash)) { + logger.warn("SAML response was not found."); + logger.debug("SAML response hash {} not fonud in response map.", responseHash); Review comment: found* ########## File path: extensions/guacamole-auth-saml/src/main/java/org/apache/guacamole/auth/saml/AuthenticationProviderService.java ########## @@ -0,0 +1,289 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.guacamole.auth.saml; + +import com.google.inject.Inject; +import com.google.inject.Provider; +import com.onelogin.saml2.authn.AuthnRequest; +import com.onelogin.saml2.authn.SamlResponse; +import com.onelogin.saml2.exception.SettingsException; +import com.onelogin.saml2.exception.ValidationError; +import com.onelogin.saml2.settings.Saml2Settings; +import com.onelogin.saml2.util.Util; +import java.io.IOException; +import java.net.URI; +import java.net.URISyntaxException; +import java.util.Arrays; +import java.util.Collections; +import java.util.HashMap; +import java.util.HashSet; +import java.util.List; +import java.util.Map; +import java.util.Map.Entry; +import java.util.Set; +import javax.servlet.http.HttpServletRequest; +import javax.xml.parsers.ParserConfigurationException; +import javax.xml.xpath.XPathExpressionException; +import org.apache.guacamole.auth.saml.conf.ConfigurationService; +import org.apache.guacamole.auth.saml.user.SAMLAuthenticatedUser; +import org.apache.guacamole.GuacamoleException; +import org.apache.guacamole.form.Field; +import org.apache.guacamole.form.RedirectField; +import org.apache.guacamole.language.TranslatableMessage; +import org.apache.guacamole.net.auth.AuthenticatedUser; +import org.apache.guacamole.net.auth.Credentials; +import org.apache.guacamole.net.auth.credentials.CredentialsInfo; +import org.apache.guacamole.net.auth.credentials.GuacamoleInvalidCredentialsException; +import org.apache.guacamole.net.auth.credentials.GuacamoleInsufficientCredentialsException; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; +import org.xml.sax.SAXException; + +/** + * Class that provides services for use by the SAMLAuthenticationProvider class. + */ +public class AuthenticationProviderService { + + /** + * Logger for this class. + */ + private final Logger logger = LoggerFactory.getLogger(AuthenticationProviderService.class); + + /** + * Service for retrieving SAML configuration information. + */ + @Inject + private ConfigurationService confService; + + /** + * Provider for AuthenticatedUser objects. + */ + @Inject + private Provider<SAMLAuthenticatedUser> authenticatedUserProvider; + + /** + * The map used to track active SAML responses. + */ + @Inject + private SAMLResponseMap samlResponseMap; + + /** + * Returns an AuthenticatedUser representing the user authenticated by the + * given credentials. + * + * @param credentials + * The credentials to use for authentication. + * + * @return + * An AuthenticatedUser representing the user authenticated by the + * given credentials. + * + * @throws GuacamoleException + * If an error occurs while authenticating the user, or if access is + * denied. + */ + public AuthenticatedUser authenticateUser(Credentials credentials) + throws GuacamoleException { + + HttpServletRequest request = credentials.getRequest(); + + // Initialize and configure SAML client. + Saml2Settings samlSettings = confService.getSamlSettings(); + + if (request != null) { + + // Look for the SAML Response parameter. + String responseHash = request.getParameter("responseHash"); + + if (responseHash != null) { + + try { + + // Generate the response object + if (!samlResponseMap.hasSamlResponse(responseHash)) { + logger.warn("SAML response was not found."); + logger.debug("SAML response hash {} not fonud in response map.", responseHash); + throw new GuacamoleInvalidCredentialsException("Provided response was not found.", + CredentialsInfo.USERNAME_PASSWORD); + } + + SamlResponse samlResponse = samlResponseMap.getSamlResponse(responseHash); + + if (!samlResponse.validateNumAssertions()) { + logger.warn("SAML response contained other than single assertion."); + logger.debug("validateNumAssertions returned false."); + throw new GuacamoleInvalidCredentialsException("Error during SAML login.", + CredentialsInfo.USERNAME_PASSWORD); + } + + // Validate timestamps, generating ValidationException if this fails. + samlResponse.validateTimestamps(); + + // Grab the username, and, if present, finish authentication. + String username = samlResponse.getNameId().toLowerCase(); + if (username != null) { + + // Retrieve any provided attributes + Map<String, List<String>> attributes = + samlResponse.getAttributes(); + + // Back-port the username to the credentials + credentials.setUsername(username); + + // Configure the AuthenticatedUser and return it + SAMLAuthenticatedUser authenticatedUser = + authenticatedUserProvider.get(); + + authenticatedUser.init(username, credentials, + parseTokens(attributes), + parseGroups(attributes, confService.getGroupAttribute())); + + return authenticatedUser; + } + } + + // Errors are logged and result in a normal username/password login box. + catch (IOException e) { + logger.warn("Error during I/O while parsing SAML response: {}", e.getMessage()); + logger.debug("Received IOException when trying to parse SAML response.", e); + throw new GuacamoleInvalidCredentialsException("Error during SAML login.", + CredentialsInfo.USERNAME_PASSWORD); + } + catch (ParserConfigurationException e) { + logger.warn("Error configuring XML parser: {}", e.getMessage()); + logger.debug("Received ParserConfigurationException when trying to parse SAML response.", e); + throw new GuacamoleInvalidCredentialsException("Error during SAML login.", + CredentialsInfo.USERNAME_PASSWORD); + } + catch (SAXException e) { + logger.warn("Bad XML when parsing SAML response: {}", e.getMessage()); + logger.debug("Received SAXException while parsing SAML response.", e); + throw new GuacamoleInvalidCredentialsException("Error during SAML login.", + CredentialsInfo.USERNAME_PASSWORD); + } + catch (SettingsException e) { + logger.warn("Error with SAML settings while parsing response: {}", e.getMessage()); + logger.debug("Received SettingsException while parsing SAML response.", e); + throw new GuacamoleInvalidCredentialsException("Error during SAML login.", + CredentialsInfo.USERNAME_PASSWORD); + } + catch (ValidationError e) { + logger.warn("Error validating SAML response: {}", e.getMessage()); + logger.debug("Received ValidationError while parsing SAML response.", e); + throw new GuacamoleInvalidCredentialsException("Error during SAML login.", + CredentialsInfo.USERNAME_PASSWORD); + } + catch (XPathExpressionException e) { + logger.warn("Problem with XML parsing response: {}", e.getMessage()); + logger.debug("Received XPathExpressionException while processing SAML response.", e); + throw new GuacamoleInvalidCredentialsException("Error during SAML login.", + CredentialsInfo.USERNAME_PASSWORD); + } + catch (Exception e) { + logger.warn("Exception while getting name from SAML response: {}", e.getMessage()); + logger.debug("Received Exception while retrieving name from SAML response.", e); + throw new GuacamoleInvalidCredentialsException("Error during SAML login.", + CredentialsInfo.USERNAME_PASSWORD); + } + } + } + + // No SAML Response is present, so generate a request. + AuthnRequest samlReq = new AuthnRequest(samlSettings); + URI authUri; + try { + authUri = new URI(samlSettings.getIdpSingleSignOnServiceUrl() + "?SAMLRequest=" + + Util.urlEncoder(samlReq.getEncodedAuthnRequest())); + } + catch (IOException e) { + logger.error("Error encoding authentication request to string: {}", e.getMessage()); + logger.debug("Got IOException encoding authentication request.", e); + throw new GuacamoleInvalidCredentialsException("Error during SAML login.", + CredentialsInfo.USERNAME_PASSWORD); + } + catch(URISyntaxException e) { + logger.error("Error generating URI for authentication redirect: {}", e.getMessage()); + logger.debug("Got URISyntaxException generating authentication URI", e); + throw new GuacamoleInvalidCredentialsException("Error during SAML login.", + CredentialsInfo.USERNAME_PASSWORD); + } + + // Redirect to SAML Identity Provider (IdP) + throw new GuacamoleInsufficientCredentialsException("Redirecting to SAML IdP.", + new CredentialsInfo(Arrays.asList(new Field[] { + new RedirectField("samlRedirect", authUri, new TranslatableMessage("LOGIN.INFO_SAML_REDIRECT_PENDING")) + })) + ); + + } + + /** + * Generates Map of tokens that can be substituted within Guacamole + * parameters given a Map containing a List of attributes from the SAML IdP. + * Attributes that have multiple values will be reduced to a single value, + * taking the first available value and discarding the remaining values. + * + * @param attributes + * The Map containing the attributes retrieved from the SAML IdP. + * + * @return + * A Map of key and single value pairs that can be used as parameter + * tokens. + */ + private Map<String, String> parseTokens(Map<String, + List<String>> attributes) { + + Map<String, String> tokens = new HashMap<>(); + for (Entry<String, List<String>> entry : attributes.entrySet()) { + + List<String> values = entry.getValue(); + tokens.put(entry.getKey(), values.get(0)); Review comment: Is it OK that these tokens will have identical names to their attributes within SAML? Or should the names be canonicalized, prefixed, etc. with http://guacamole.apache.org/doc/guacamole-ext/org/apache/guacamole/token/TokenName.html#canonicalize-java.lang.String-java.lang.String-? ---------------------------------------------------------------- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: [email protected]
