You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@guacamole.apache.org by GitBox <gi...@apache.org> on 2021/11/22 06:32:51 UTC
[GitHub] [guacamole-client] mike-jumper opened a new pull request #653: GUACAMOLE-1364: Clean up overall logic of SAML authentication flow.
mike-jumper opened a new pull request #653:
URL: https://github.com/apache/guacamole-client/pull/653
In preparation for the SSO auth flow changes of [GUACAMOLE-1364](https://issues.apache.org/jira/browse/GUACAMOLE-1364), and in anticipation of future SAML changes like [GUACAMOLE-1372](https://issues.apache.org/jira/browse/GUACAMOLE-1372), this change cleans up the logic of the overall SAML authentication flow.
All handling of the actual SAML process is now abstracted away into several smaller pieces that are more narrow in scope:
* `AuthenticationSession`: An internal representation of the continuing SAML authentication process as it spans multiple requests and redirects.
* `AssertedIdentity`: An internal representation of the SAML assertion.
* `SAMLService`: A service which abstracts away all SAML response/request handling (so the Guacamole-specific pieces don't have to)
The general goal of this is to reduce and clarify the responsibility of each class to the extent that future changes will be more obvious and verifiable, particularly changes that touch the semi-complex flow that was involved with SAML. A happy side-effect of these changes is that logging is also a bit more clear.
As I propagated the refactor through, it became clear that the parameter token functionality added to the SAML support is not actually used anywhere (`getTokens()` is not invoked). I'm not sure if this has always been the case, but rather than simply apply a refactor to code that has no effect (and thus isn't testable), this also addresses that.
--
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.
To unsubscribe, e-mail: dev-unsubscribe@guacamole.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [guacamole-client] necouchman commented on a change in pull request #653: GUACAMOLE-1364: Clean up overall logic of SAML authentication flow.
Posted by GitBox <gi...@apache.org>.
necouchman commented on a change in pull request #653:
URL: https://github.com/apache/guacamole-client/pull/653#discussion_r754533824
##########
File path: extensions/guacamole-auth-saml/src/main/java/org/apache/guacamole/auth/saml/AuthenticationProviderService.java
##########
@@ -21,71 +21,51 @@
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 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.ws.rs.core.UriBuilder;
-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.GuacamoleServerException;
+import org.apache.guacamole.auth.saml.acs.AssertedIdentity;
+import org.apache.guacamole.auth.saml.acs.AuthenticationSessionManager;
+import org.apache.guacamole.auth.saml.acs.SAMLService;
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.GuacamoleInsufficientCredentialsException;
-import org.apache.guacamole.token.TokenName;
-import org.slf4j.Logger;
-import org.slf4j.LoggerFactory;
-import org.xml.sax.SAXException;
/**
- * Class that provides services for use by the SAMLAuthenticationProvider class.
+ * Service that authenticates Guacamole users by processing the responses of
+ * SAML identity providers.
*/
public class AuthenticationProviderService {
/**
- * Logger for this class.
+ * The name of the query parameter that identifies an active authentication
+ * session (in-progress SAML authentication attempt).
*/
- private static final Logger logger = LoggerFactory.getLogger(AuthenticationProviderService.class);
+ public static final String AUTH_SESSION_QUERY_PARAM = "state";
/**
- * Service for retrieving SAML configuration information.
+ * Provider for AuthenticatedUser objects.
*/
@Inject
- private ConfigurationService confService;
+ private Provider<SAMLAuthenticatedUser> authenticatedUserProvider;
/**
- * Provider for AuthenticatedUser objects.
+ * Manager of active SAML authentication attempts.
*/
@Inject
- private Provider<SAMLAuthenticatedUser> authenticatedUserProvider;
-
+ private AuthenticationSessionManager authManager;
Review comment:
This is a complete #NitPick, but this is called `authManager`, here, and `sessionManager` in the `SAMLService` class - unless there's a compelling reason for naming them differently, might be best to keep them the same?
##########
File path: extensions/guacamole-auth-saml/src/main/java/org/apache/guacamole/auth/saml/acs/AuthenticationSession.java
##########
@@ -0,0 +1,113 @@
+/*
+ * 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.acs;
+
+/**
+ * Representation of an in-progress SAML authentication attempt.
+ */
+public class AuthenticationSession {
+
+ /**
+ * The absolute point in time after which this authentication session is
+ * invalid. This value is a UNIX epoch timestamp, as may be returned by
+ * {@link System#currentTimeMillis()}.
+ */
+ private final long expirationTimestamp;
+
+ /**
+ * The request ID of the SAML request associated with the authentication
+ * attempt.
+ */
+ private final String requestId;
+
+ /**
+ * The identity asserted by the SAML IdP, or null if authentication has not
+ * yet completed successfully.
+ */
+ private AssertedIdentity identity = null;
+
+ /**
+ * Creates a new AuthenticationSession representing an in-progress SAML
+ * authentication attempt.
+ *
+ * @param requestId
+ * The request ID of the SAML request associated with the
+ * authentication attempt.
+ *
+ * @param expires
+ * The number of milliseconds that may elapse before this session must
+ * be considered invalid.
+ */
+ public AuthenticationSession(String requestId, long expires) {
+ this.expirationTimestamp = System.currentTimeMillis() + expires;
+ this.requestId = requestId;
+ }
+
+ /**
+ * Returns whether this authentication session is still valid (has not yet
+ * expired). If an identity has been asserted by the SAML IdP, this
+ * considers also whether the SAML response asserting that identity has
+ * expired.
+ *
+ * @return
+ * true if this authentication session is still valid, false if it has
+ * expired.
+ */
+ public boolean isValid() {
+ return System.currentTimeMillis() < expirationTimestamp
+ && (identity == null || identity.isValid());
Review comment:
So, the session is considered valid if it has not, yet, expired, and the identity is null? That is, it is valid if authentication is still in progress? Just want to make sure that's what is intended...
--
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.
To unsubscribe, e-mail: dev-unsubscribe@guacamole.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [guacamole-client] mike-jumper commented on a change in pull request #653: GUACAMOLE-1364: Clean up overall logic of SAML authentication flow.
Posted by GitBox <gi...@apache.org>.
mike-jumper commented on a change in pull request #653:
URL: https://github.com/apache/guacamole-client/pull/653#discussion_r754613179
##########
File path: extensions/guacamole-auth-saml/src/main/java/org/apache/guacamole/auth/saml/AuthenticationProviderService.java
##########
@@ -21,71 +21,51 @@
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 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.ws.rs.core.UriBuilder;
-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.GuacamoleServerException;
+import org.apache.guacamole.auth.saml.acs.AssertedIdentity;
+import org.apache.guacamole.auth.saml.acs.AuthenticationSessionManager;
+import org.apache.guacamole.auth.saml.acs.SAMLService;
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.GuacamoleInsufficientCredentialsException;
-import org.apache.guacamole.token.TokenName;
-import org.slf4j.Logger;
-import org.slf4j.LoggerFactory;
-import org.xml.sax.SAXException;
/**
- * Class that provides services for use by the SAMLAuthenticationProvider class.
+ * Service that authenticates Guacamole users by processing the responses of
+ * SAML identity providers.
*/
public class AuthenticationProviderService {
/**
- * Logger for this class.
+ * The name of the query parameter that identifies an active authentication
+ * session (in-progress SAML authentication attempt).
*/
- private static final Logger logger = LoggerFactory.getLogger(AuthenticationProviderService.class);
+ public static final String AUTH_SESSION_QUERY_PARAM = "state";
/**
- * Service for retrieving SAML configuration information.
+ * Provider for AuthenticatedUser objects.
*/
@Inject
- private ConfigurationService confService;
+ private Provider<SAMLAuthenticatedUser> authenticatedUserProvider;
/**
- * Provider for AuthenticatedUser objects.
+ * Manager of active SAML authentication attempts.
*/
@Inject
- private Provider<SAMLAuthenticatedUser> authenticatedUserProvider;
-
+ private AuthenticationSessionManager authManager;
Review comment:
Fixed via rebase.
--
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.
To unsubscribe, e-mail: dev-unsubscribe@guacamole.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [guacamole-client] mike-jumper commented on a change in pull request #653: GUACAMOLE-1364: Clean up overall logic of SAML authentication flow.
Posted by GitBox <gi...@apache.org>.
mike-jumper commented on a change in pull request #653:
URL: https://github.com/apache/guacamole-client/pull/653#discussion_r754611793
##########
File path: extensions/guacamole-auth-saml/src/main/java/org/apache/guacamole/auth/saml/acs/AuthenticationSession.java
##########
@@ -0,0 +1,113 @@
+/*
+ * 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.acs;
+
+/**
+ * Representation of an in-progress SAML authentication attempt.
+ */
+public class AuthenticationSession {
+
+ /**
+ * The absolute point in time after which this authentication session is
+ * invalid. This value is a UNIX epoch timestamp, as may be returned by
+ * {@link System#currentTimeMillis()}.
+ */
+ private final long expirationTimestamp;
+
+ /**
+ * The request ID of the SAML request associated with the authentication
+ * attempt.
+ */
+ private final String requestId;
+
+ /**
+ * The identity asserted by the SAML IdP, or null if authentication has not
+ * yet completed successfully.
+ */
+ private AssertedIdentity identity = null;
+
+ /**
+ * Creates a new AuthenticationSession representing an in-progress SAML
+ * authentication attempt.
+ *
+ * @param requestId
+ * The request ID of the SAML request associated with the
+ * authentication attempt.
+ *
+ * @param expires
+ * The number of milliseconds that may elapse before this session must
+ * be considered invalid.
+ */
+ public AuthenticationSession(String requestId, long expires) {
+ this.expirationTimestamp = System.currentTimeMillis() + expires;
+ this.requestId = requestId;
+ }
+
+ /**
+ * Returns whether this authentication session is still valid (has not yet
+ * expired). If an identity has been asserted by the SAML IdP, this
+ * considers also whether the SAML response asserting that identity has
+ * expired.
+ *
+ * @return
+ * true if this authentication session is still valid, false if it has
+ * expired.
+ */
+ public boolean isValid() {
+ return System.currentTimeMillis() < expirationTimestamp
+ && (identity == null || identity.isValid());
Review comment:
Yep - exactly. If no identity has yet been asserted, the only factor in session validity is whether Guacamole itself still considers the session valid. Once an identity has been asserted, then both Guacamole and the SAML response are taken into account.
--
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.
To unsubscribe, e-mail: dev-unsubscribe@guacamole.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [guacamole-client] mike-jumper commented on a change in pull request #653: GUACAMOLE-1364: Clean up overall logic of SAML authentication flow.
Posted by GitBox <gi...@apache.org>.
mike-jumper commented on a change in pull request #653:
URL: https://github.com/apache/guacamole-client/pull/653#discussion_r754612050
##########
File path: extensions/guacamole-auth-saml/src/main/java/org/apache/guacamole/auth/saml/AuthenticationProviderService.java
##########
@@ -21,71 +21,51 @@
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 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.ws.rs.core.UriBuilder;
-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.GuacamoleServerException;
+import org.apache.guacamole.auth.saml.acs.AssertedIdentity;
+import org.apache.guacamole.auth.saml.acs.AuthenticationSessionManager;
+import org.apache.guacamole.auth.saml.acs.SAMLService;
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.GuacamoleInsufficientCredentialsException;
-import org.apache.guacamole.token.TokenName;
-import org.slf4j.Logger;
-import org.slf4j.LoggerFactory;
-import org.xml.sax.SAXException;
/**
- * Class that provides services for use by the SAMLAuthenticationProvider class.
+ * Service that authenticates Guacamole users by processing the responses of
+ * SAML identity providers.
*/
public class AuthenticationProviderService {
/**
- * Logger for this class.
+ * The name of the query parameter that identifies an active authentication
+ * session (in-progress SAML authentication attempt).
*/
- private static final Logger logger = LoggerFactory.getLogger(AuthenticationProviderService.class);
+ public static final String AUTH_SESSION_QUERY_PARAM = "state";
/**
- * Service for retrieving SAML configuration information.
+ * Provider for AuthenticatedUser objects.
*/
@Inject
- private ConfigurationService confService;
+ private Provider<SAMLAuthenticatedUser> authenticatedUserProvider;
/**
- * Provider for AuthenticatedUser objects.
+ * Manager of active SAML authentication attempts.
*/
@Inject
- private Provider<SAMLAuthenticatedUser> authenticatedUserProvider;
-
+ private AuthenticationSessionManager authManager;
Review comment:
That's fair - I think this naming is from an earlier stage in the refactor. I'll update it to be consistent.
--
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.
To unsubscribe, e-mail: dev-unsubscribe@guacamole.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [guacamole-client] necouchman merged pull request #653: GUACAMOLE-1364: Clean up overall logic of SAML authentication flow.
Posted by GitBox <gi...@apache.org>.
necouchman merged pull request #653:
URL: https://github.com/apache/guacamole-client/pull/653
--
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.
To unsubscribe, e-mail: dev-unsubscribe@guacamole.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org