-
Notifications
You must be signed in to change notification settings - Fork 694
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-1289: Resolve relogin issue, remove spring dependency from Duo Auth, and update dependencies with vulnerabilities. #973
Conversation
f064ec0
to
6708115
Compare
Added a fix for the relogin bug mentioned in the previous Pull request #966 (comment) |
guacamole-ext/src/main/java/org/apache/guacamole/net/auth/Credentials.java
Outdated
Show resolved
Hide resolved
|
6708115
to
b91dfe9
Compare
…cy and update dependencies with vulnerabilities.
b91dfe9
to
282a911
Compare
* | ||
* @return True if authentication is resumed, otherwise false. | ||
*/ | ||
public Boolean isAuthenticationResumed() { |
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.
It'd be nice if this could avoid going in Credentials
, because it's pretty specific to Duo, and it's also not even the only way of resuming sessions - see the SSO (SAML, etc) extensions, which have their own completely-different mechanism for resuming interrupted auth attempts.
Maybe the DUO extension could do something similar? (using a URL parameter to track state). If there could be some shared functionality for this, it would be ideal, though that seems unlikely considering how different the implementations are.
At the bare minimum, I think that the SSO extensions should at least set this flag.
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.
The idea was to migrate sso to this method
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.
It looks like the DUO extension is already tracking state through URL parameters.
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.
I tried to create a generic system for resumable authentication based on query strings set by the authentication provider. The query strings are defined by the authentication provider modules so that we can avoid accidental overlap.
Lines 57 to 61 in 282a911
/** | |
* The name of the parameter that will be used in the GET call-back that | |
* contains the session state. | |
*/ | |
public static final String DUO_STATE_PARAMETER_NAME = "state"; |
Here is the order of events:
- The user attempts to authenticate with the primary application.
- If additional authentication is needed (e.g. Duo), the user is temporarily redirected to an external service.
- The primary application throws an error containing the necessary information to resume the authentication session and redirects to the external authentication service.
Lines 126 to 138 in 282a911
throw new TranslatableGuacamoleInsufficientCredentialsException( "Verification using Duo is required before authentication " + "can continue.", "LOGIN.INFO_DUO_AUTH_REQUIRED", new CredentialsInfo(Collections.singletonList( new RedirectField( DUO_CODE_PARAMETER_NAME, new URI(duoClient.createAuthUrl(username, duoState)), new TranslatableMessage("LOGIN.INFO_DUO_REDIRECT_PENDING") ) )), duoState, DuoAuthenticationProvider.PROVIDER_IDENTIFER, DUO_STATE_PARAMETER_NAME, expirationTimestamp ); - The primary application catches the error, stores the relevant information in a map for retrieval, and marks the credentials so that we know they need to be resumed.
guacamole-client/guacamole/src/main/java/org/apache/guacamole/rest/auth/AuthenticationService.java
Lines 327 to 334 in 282a911
String state = e.getState(); long expiration = e.getExpires(); String queryIdentifier = e.getQueryIdentifier(); String providerIdentifier = e.getProviderIdentifier(); credentials.setAuthenticationResumed(true); resumableStateMap.put(state, new ResumableAuthenticationState(providerIdentifier, queryIdentifier, expiration, credentials)); - The external authentication service performs its task (verifying the user's identity) and then redirects the user back to the primary application with special query string parameters that indicate the result of the authentication process.
- The primary application handles the redirected request. It checks the query string parameters for a specific state parameter that matches a known "resumable state."
guacamole-client/guacamole/src/main/java/org/apache/guacamole/rest/auth/AuthenticationService.java
Lines 370 to 433 in 282a911
private Credentials resumeAuthentication(Credentials credentials) { Credentials resumedCredentials = null; // Retrieve signed State from the request HttpServletRequest request = credentials.getRequest(); // Retrieve the provider id from the query parameters String resumableProviderId = request.getParameter(Credentials.RESUME_QUERY); // Check if a provider id is set if (resumableProviderId == null || resumableProviderId.isEmpty()) { // Return if a provider id is not set return null; } // Use an iterator to safely remove entries while iterating Iterator<Map.Entry<String, ResumableAuthenticationState>> iterator = resumableStateMap.entrySet().iterator(); while (iterator.hasNext()) { Map.Entry<String, ResumableAuthenticationState> entry = iterator.next(); ResumableAuthenticationState resumableState = entry.getValue(); // Check if the provider ID from the request matches the one in the map entry boolean providerMatches = resumableProviderId.equals(resumableState.getProviderIdentifier()); if (!providerMatches) { // If the provider doesn't match, skip to the next entry continue; } // Use the query identifier from the entry to retrieve the corresponding state parameter String stateQueryParameter = resumableState.getQueryIdentifier(); String stateFromParameter = request.getParameter(stateQueryParameter); // Check if a state parameter is set if (stateFromParameter == null || stateFromParameter.isEmpty()) { // Remove and continue if`state is not provided or is empty iterator.remove(); continue; } // If the key in the entry (state) matches the state parameter provided in the request if (entry.getKey().equals(stateFromParameter)) { // Remove the current entry from the map iterator.remove(); // Check if the resumableState has expired if (!resumableState.isExpired()) { // Set the actualCredentials to the credentials from the matched entry resumedCredentials = resumableState.getCredentials(); if (resumedCredentials != null) { resumedCredentials.setRequest(request); } } // Exit the loop since we've found the matching state and it's unique break; } } return resumedCredentials; } - On the primary application side, these states, along with information such as provider identifiers and credentials, are stored in a map. This allows the application to keep track of which authentication state corresponds to which user session.
- If a matching state is found and the state has not expired, the primary application uses the stored credentials and information to resume the user's authentication process.
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.
The query strings are defined by the authentication provider modules so that we can avoid accidental overlap
FWIW, the ULR parameter for SAML is also called "state". Likely things won't work if people try to use DUO + SAML together. Might want to consider changing one or both of those.
Were you planning on migrating the SSO extensions to use this new flag in this PR?
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.
You're correct that the SAML extension also uses a state
parameter, which could lead to confusion when both DUO
and SAML
are used concurrently, but I also added the provider_id
as a required query parameter for resuming authentication. We don't have control over the name of the state
query parameter received from DUO
so I figured it was best to have the provider_id
act as a namespacing mechanism to prevent collision. When redirecting from duo the query parameters will look like such: provider_id=duo&state=abc123&duo_code=abc123
.
This PR isn't meant for migrating the SSO extensions but their was the plan to eventually migrate them to use the same code for resuming authentication.
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.
It definitely feels like we shouldn't have the SSO extensions unmigrated for very long ... what do you think @mike-jumper ? Should migration of the SSO extensions to this new system be part of this ticket?
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.
I would agree that the SSO extension migration should be with this ticket - either this PR, or another PR to follow quickly after this one...
Closing in favor of recently-merged #980. |
The merge of corresponding upstream #966 to main resulted in a bunch of dependabot warnings over old, vulnerable dependencies getting pulled in:
![image](https://private-user-images.githubusercontent.com/7276007/321438472-c20d2c2c-c373-42a3-9083-338a98db1c1f.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MjQ3MzgyMzEsIm5iZiI6MTcyNDczNzkzMSwicGF0aCI6Ii83Mjc2MDA3LzMyMTQzODQ3Mi1jMjBkMmMyYy1jMzczLTQyYTMtOTA4My0zMzhhOThkYjFjMWYucG5nP1gtQW16LUFsZ29yaXRobT1BV1M0LUhNQUMtU0hBMjU2JlgtQW16LUNyZWRlbnRpYWw9QUtJQVZDT0RZTFNBNTNQUUs0WkElMkYyMDI0MDgyNyUyRnVzLWVhc3QtMSUyRnMzJTJGYXdzNF9yZXF1ZXN0JlgtQW16LURhdGU9MjAyNDA4MjdUMDU1MjExWiZYLUFtei1FeHBpcmVzPTMwMCZYLUFtei1TaWduYXR1cmU9YTI2NzY2YzYwNzI4NTgwNjE3YjU2OWEzZjMyM2QzZGRiMmVmYTg0ZWFkOGE1N2Q3MTZiMmU2MGViM2Q1Nzg2MCZYLUFtei1TaWduZWRIZWFkZXJzPWhvc3QmYWN0b3JfaWQ9MCZrZXlfaWQ9MCZyZXBvX2lkPTAifQ.PRmz06znlOgYq3xvCb4fdwUwBSFPjUyfA7bGYSfNme8)
There is also an issue where logging out and then logging back in would result in an error thrown. This was due to query strings being checked to see if we were resuming authentication or not. I have added a new variable that is tracked within the
Credentials
to know if we are actually resuming authentication.We also removed the spring requirement from Duo Auth when building a uri with specific query strings.