You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@guacamole.apache.org by "aleitner (via GitHub)" <gi...@apache.org> on 2024/03/29 00:44:30 UTC
[PR] GUACAMOLE-1289: Duo upgrade to API v4 [guacamole-client]
aleitner opened a new pull request, #966:
URL: https://github.com/apache/guacamole-client/pull/966
I continued working off @necouchman's PR https://github.com/apache/guacamole-client/pull/915 to add support for resuming authentication when dealing with the redirect authentication model
--
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
Re: [PR] GUACAMOLE-1289: Duo upgrade to API v4 [guacamole-client]
Posted by "jmuehlner (via GitHub)" <gi...@apache.org>.
jmuehlner commented on code in PR #966:
URL: https://github.com/apache/guacamole-client/pull/966#discussion_r1550648442
##########
extensions/guacamole-auth-duo/src/main/java/org/apache/guacamole/auth/duo/conf/ConfigurationService.java:
##########
@@ -56,11 +59,11 @@ public class ConfigurationService {
* key received from Duo for verifying Guacamole users. This value MUST be
* exactly 20 characters.
*/
- private static final StringGuacamoleProperty DUO_INTEGRATION_KEY =
Review Comment:
It looks like this block comment should also be updated.
--
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
Re: [PR] GUACAMOLE-1289: Duo upgrade to API v4 [guacamole-client]
Posted by "stcbus (via GitHub)" <gi...@apache.org>.
stcbus commented on PR #966:
URL: https://github.com/apache/guacamole-client/pull/966#issuecomment-2038042175
Also, just to note (I'm sure it's known!), the docker `start.sh` script needs updated with the new names of the DUO keys and to incorporate the new variables for redirect URL and timeout.
--
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
Re: [PR] GUACAMOLE-1289: Duo upgrade to API v4 [guacamole-client]
Posted by "mike-jumper (via GitHub)" <gi...@apache.org>.
mike-jumper commented on PR #966:
URL: https://github.com/apache/guacamole-client/pull/966#issuecomment-2037924017
> Not sure if this is the correct place to mention this or not, but using this branch works perfectly for me, except on the redirect to Duo, I just see a "LOGIN.INFO_DUO_REDIRECT_PENDING" text. I assume it is missing the translation text for that variable in translations?
Yes, that string will need to be added here. I don't see any changes present that define that string.
If you still see that text after the string is added, you probably just need to clear browser cache.
--
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
Re: [PR] GUACAMOLE-1289: Duo upgrade to API v4 [guacamole-client]
Posted by "necouchman (via GitHub)" <gi...@apache.org>.
necouchman commented on PR #966:
URL: https://github.com/apache/guacamole-client/pull/966#issuecomment-2027366576
@stcbus Everything is in this PR - you don't need to pull both of them.
--
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
Re: [PR] GUACAMOLE-1289: Duo upgrade to API v4 [guacamole-client]
Posted by "stcbus (via GitHub)" <gi...@apache.org>.
stcbus commented on PR #966:
URL: https://github.com/apache/guacamole-client/pull/966#issuecomment-2050816886
Let me know if I should report a bug now, but since it's not released quite yet, we just ran into 2 issues I think that maybe are things that need fixed for the extension:
1. I am getting failures after successful logins, and that can trigger the ban extension now with lots of testing. I don't believe we had that before, so I think maybe somehow Duo extension doesn't report correctly to the ban extension?
2. In a certain scenario, we get a generic guacamole error occured message. This is 100% reproducible in the following scenario:
- Log into guac, pass through duo
- after log in, log out
- click "relogin" button
Re-loading the page is fine, so something with the relogin button or flow from there, we are rejected and get the below log message:
`01:35:12.356 [http-nio-8080-exec-5] WARN o.a.g.event.EventLoggingListener - Authentication attempt from 128.146.93.48 for user "USERNAME failed: Duo Client error. (rejected by "duo")
01:35:12.356 [http-nio-8080-exec-5] ERROR o.a.g.rest.RESTExceptionMapper - Request could not be processed: Duo Client error.`
--
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
Re: [PR] GUACAMOLE-1289: Duo upgrade to API v4 [guacamole-client]
Posted by "jmuehlner (via GitHub)" <gi...@apache.org>.
jmuehlner commented on code in PR #966:
URL: https://github.com/apache/guacamole-client/pull/966#discussion_r1550652174
##########
guacamole-ext/src/main/java/org/apache/guacamole/net/auth/credentials/GuacamoleInsufficientCredentialsException.java:
##########
@@ -28,6 +28,95 @@
*/
public class GuacamoleInsufficientCredentialsException extends GuacamoleCredentialsException {
+/**
+ * The default state token to use when no specific state information is provided.
+ */
+private static final String DEFAULT_STATE = "";
+
+/**
+ * The default provider identifier to use when no specific provider is identified.
+ * This serves as a placeholder indicating that either no specific provider is
+ * responsible for the exception or the responsible provider has not been identified.
+ */
+private static final String DEFAULT_PROVIDER_IDENTIFIER = "";
+
+/**
+ * The default query identifier to use when no specific query is identified.
+ * This serves as a placeholder and indicates that the specific query related to
+ * the provider's state resume operation has not been provided.
+ */
+private static final String DEFAULT_QUERY_IDENTIFIER = "";
+
+/**
+ * The default expiration timestamp to use when no specific expiration is provided,
+ * effectively indicating that the state token does not expire.
+ */
+private static final long DEFAULT_EXPIRES = -1L;
+
+/**
+ * An opaque value that may be used by a client to maintain state across requests
+ * which are part of the same authentication transaction.
+ */
+protected final String state;
+
+ /**
+ * The identifier for the authentication provider that threw this exception.
+ * This is used to link the exception back to the originating source of the
+ * authentication attempt, allowing clients to determine which provider's
+ * authentication process should be resumed.
+ */
+ protected final String providerIdentifier;
Review Comment:
These fields look overindented.
--
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
Re: [PR] GUACAMOLE-1289: Duo upgrade to API v4 [guacamole-client]
Posted by "jmuehlner (via GitHub)" <gi...@apache.org>.
jmuehlner commented on code in PR #966:
URL: https://github.com/apache/guacamole-client/pull/966#discussion_r1550659212
##########
extensions/guacamole-auth-duo/src/main/java/org/apache/guacamole/auth/duo/conf/ConfigurationService.java:
##########
@@ -69,26 +72,38 @@ public class ConfigurationService {
* received from Duo for verifying Guacamole users. This value MUST be
* exactly 40 characters.
*/
- private static final StringGuacamoleProperty DUO_SECRET_KEY =
+ private static final StringGuacamoleProperty DUO_CLIENT_SECRET =
new StringGuacamoleProperty() {
@Override
- public String getName() { return "duo-secret-key"; }
+ public String getName() { return "duo-client-secret"; }
};
-
+
Review Comment:
No bogus whitespace please.
##########
extensions/guacamole-auth-duo/src/main/java/org/apache/guacamole/auth/duo/conf/ConfigurationService.java:
##########
@@ -110,51 +125,65 @@ public String getAPIHostname() throws GuacamoleException {
}
/**
- * Returns the integration key received from Duo for verifying Guacamole
- * users, as defined in guacamole.properties by the "duo-integration-key"
+ * Returns the Duo client id received from Duo for verifying Guacamole
+ * users, as defined in guacamole.properties by the "duo-client-id"
* property. This value MUST be exactly 20 characters.
*
* @return
- * The integration key received from Duo for verifying Guacamole
- * users.
+ * The client id received from Duo for verifying Guacamole users.
*
* @throws GuacamoleException
* If the associated property within guacamole.properties is missing.
*/
- public String getIntegrationKey() throws GuacamoleException {
- return environment.getRequiredProperty(DUO_INTEGRATION_KEY);
+ public String getClientId() throws GuacamoleException {
+ return environment.getRequiredProperty(DUO_CLIENT_ID);
}
/**
- * Returns the secret key received from Duo for verifying Guacamole users,
- * as defined in guacamole.properties by the "duo-secret-key" property. This
- * value MUST be exactly 20 characters.
+ * Returns the client secert received from Duo for verifying Guacamole users,
+ * as defined in guacamole.properties by the "duo-client-secert" property.
Review Comment:
Typo: `secert`
--
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
Re: [PR] GUACAMOLE-1289: Duo upgrade to API v4 [guacamole-client]
Posted by "aleitner (via GitHub)" <gi...@apache.org>.
aleitner commented on PR #966:
URL: https://github.com/apache/guacamole-client/pull/966#issuecomment-2038570604
Added the missing translations and revised the guacamole.properties setup for duo
--
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
Re: [PR] GUACAMOLE-1289: Duo upgrade to API v4 [guacamole-client]
Posted by "aleitner (via GitHub)" <gi...@apache.org>.
aleitner commented on PR #966:
URL: https://github.com/apache/guacamole-client/pull/966#issuecomment-2038584056
> Thanks @aleitner ! In the interest of accuracy, I believe the application key is no longer used with v4, so that part can be removed from docker/config as well. Please correct, if wrong (I certainly don't pass it in my testing).
Yup, it's no longer necessary so I just removed it. Thanks!
--
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
Re: [PR] GUACAMOLE-1289: Duo upgrade to API v4 [guacamole-client]
Posted by "necouchman (via GitHub)" <gi...@apache.org>.
necouchman commented on PR #966:
URL: https://github.com/apache/guacamole-client/pull/966#issuecomment-2026421230
Nice @aleitner - thanks for picking this up and taking it over the line!
--
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
Re: [PR] GUACAMOLE-1289: Duo upgrade to API v4 [guacamole-client]
Posted by "jmuehlner (via GitHub)" <gi...@apache.org>.
jmuehlner merged PR #966:
URL: https://github.com/apache/guacamole-client/pull/966
--
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
Re: [PR] GUACAMOLE-1289: Duo upgrade to API v4 [guacamole-client]
Posted by "aleitner (via GitHub)" <gi...@apache.org>.
aleitner commented on PR #966:
URL: https://github.com/apache/guacamole-client/pull/966#issuecomment-2050866172
> From a quick troubleshooting session, it seems like the re-login button sends you back with the duo token stuff still in the URL field? Maybe that is why?
>
> edit: Also confirmed, the duo token and other query parmeters are there right on log on as well. Positive this is it.
Looking into a fix and I'll add it to my other PR https://github.com/apache/guacamole-client/pull/973
--
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
Re: [PR] GUACAMOLE-1289: Duo upgrade to API v4 [guacamole-client]
Posted by "jmuehlner (via GitHub)" <gi...@apache.org>.
jmuehlner commented on code in PR #966:
URL: https://github.com/apache/guacamole-client/pull/966#discussion_r1550641716
##########
extensions/guacamole-auth-duo/src/main/java/org/apache/guacamole/auth/duo/UserVerificationService.java:
##########
@@ -75,39 +97,72 @@ public void verifyAuthenticatedUser(AuthenticatedUser authenticatedUser)
// Ignore anonymous users
if (authenticatedUser.getIdentifier().equals(AuthenticatedUser.ANONYMOUS_IDENTIFIER))
return;
-
- // Retrieve signed Duo response from request
- String signedResponse = request.getParameter(DuoSignedResponseField.PARAMETER_NAME);
-
- // If no signed response, request one
- if (signedResponse == null) {
-
- // Create field which requests a signed response from Duo that
- // verifies the identity of the given user via the configured
- // Duo API endpoint
- Field signedResponseField = new DuoSignedResponseField(
- confService.getAPIHostname(),
- duoService.createSignedRequest(authenticatedUser));
-
- // Create an overall description of the additional credentials
- // required to verify identity
- CredentialsInfo expectedCredentials = new CredentialsInfo(
- Collections.singletonList(signedResponseField));
+
+ String username = authenticatedUser.getIdentifier();
+
+ try {
+
+ String redirectUrl = confService.getRedirectUrl().toString();
Review Comment:
It looks like this whole block is missing a level of indentation.
--
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
Re: [PR] GUACAMOLE-1289: Duo upgrade to API v4 [guacamole-client]
Posted by "stcbus (via GitHub)" <gi...@apache.org>.
stcbus commented on PR #966:
URL: https://github.com/apache/guacamole-client/pull/966#issuecomment-2037850529
Not sure if this is the correct place to mention this or not, but using this branch works perfectly for me, except on the redirect to Duo, I just see a "LOGIN.INFO_DUO_REDIRECT_PENDING" text. I assume it is missing the translation text for that variable in translations?
--
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
Re: [PR] GUACAMOLE-1289: Duo upgrade to API v4 [guacamole-client]
Posted by "jmuehlner (via GitHub)" <gi...@apache.org>.
jmuehlner commented on code in PR #966:
URL: https://github.com/apache/guacamole-client/pull/966#discussion_r1550655669
##########
guacamole/src/main/java/org/apache/guacamole/rest/auth/AuthenticationService.java:
##########
@@ -328,6 +354,82 @@ private List<DecoratedUserContext> getUserContexts(GuacamoleSession existingSess
return userContexts;
}
+
+ /**
+ * Resumes authentication using given credentials if a matching resumable
+ * state is found.
+ *
+ * @param credentials
+ * The initial credentials containing the request object.
+ *
+ * @return
+ * Resumed credentials if a valid resumable state is found; otherwise,
+ * returns {@code null}.
+ */
+ 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.
Review Comment:
The formatting of these single-line comments is a bit funky - comments like this should always start with a capital letter (this one doesn't), and generally do not end with a period.
This comment should definitely be capitalized. I'd also consider removing the periods to match existing comment style.
--
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
Re: [PR] GUACAMOLE-1289: Duo upgrade to API v4 [guacamole-client]
Posted by "stcbus (via GitHub)" <gi...@apache.org>.
stcbus commented on PR #966:
URL: https://github.com/apache/guacamole-client/pull/966#issuecomment-2050827746
From a quick troubleshooting session, it seems like the re-login button sends you back with the duo token stuff still in the URL field? Maybe that is why?
--
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
Re: [PR] GUACAMOLE-1289: Duo upgrade to API v4 [guacamole-client]
Posted by "stcbus (via GitHub)" <gi...@apache.org>.
stcbus commented on PR #966:
URL: https://github.com/apache/guacamole-client/pull/966#issuecomment-2027353390
@aleitner great news! We are very much looking forward to this, so for testing, do I need to pull from @necouchman 's PR as well or is just pulling from your branch good?
--
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
Re: [PR] GUACAMOLE-1289: Duo upgrade to API v4 [guacamole-client]
Posted by "jmuehlner (via GitHub)" <gi...@apache.org>.
jmuehlner commented on code in PR #966:
URL: https://github.com/apache/guacamole-client/pull/966#discussion_r1550645017
##########
guacamole/src/main/java/org/apache/guacamole/rest/auth/ResumableAuthenticationState.java:
##########
@@ -0,0 +1,128 @@
+/*
+ * 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.rest.auth;
+
+import org.apache.guacamole.net.auth.Credentials;
+
+/**
+ * Encapsulates the state information required for resuming an authentication
+ * process. This includes an expiration timestamp to determine state validity
+ * and the original credentials submitted by the user.
+ */
+public class ResumableAuthenticationState {
+
+ /**
+ * The timestamp at which this state should no longer be considered valid,
+ * measured in milliseconds since the Unix epoch.
+ */
+ private long expirationTimestamp;
+
+ /**
+ * The original user credentials that were submitted at the start of the
+ * authentication process.
+ */
+ private Credentials credentials;
+
+ /**
+ * A unique string identifying the authentication provider related to the state.
+ * This field allows the client to know which provider's authentication process
+ * should be resumed using this state.
+ */
+ private String providerIdentifier;
+
+ /**
+ * A unique string that can be used to identify a specific query within the
+ * authentication process for the identified provider. This identifier can
+ * help the resumption of an authentication process.
+ */
+ private String queryIdentifier;
+
+ /**
+ * Constructs a new ResumableAuthenticationState object with the specified
+ * expiration timestamp and user credentials.
+ *
+ * @param providerIdentifier
+ * The identifier of the authentication provider to which this resumable state pertains.
+ *
+ * @param queryIdenifier
+ * The identifier of the specific query within the provider's
+ * authentication process that this state corresponds to.
+ *
+ * @param expirationTimestamp
+ * The timestamp in milliseconds since the Unix epoch when this state
+ * expires and can no longer be used to resume authentication.
+ *
+ * @param credentials
+ * The Credentials object initially submitted by the user and associated
+ * with this resumable state.
+ */
+ public ResumableAuthenticationState(String providerIdentifier, String queryIdentifier,
+ long expirationTimestamp, Credentials credentials) {
+ this.expirationTimestamp = expirationTimestamp;
+ this.credentials = credentials;
+ this.providerIdentifier = providerIdentifier;
+ this.queryIdentifier = queryIdentifier;
+ }
+
+ /**
+ * Checks if this resumable state has expired based on the stored expiration
+ * timestamp and the current system time.
+ *
+ * @return
+ * True if the current system time is after the expiration timestamp,
+ * indicating that the state is expired; false otherwise.
+ */
+ public boolean isExpired() {
+ return System.currentTimeMillis() > expirationTimestamp;
Review Comment:
Technically this should probably be `>=` here since the documentation for the `expirationTimestamp` parameter specifies that it's the time at which the state expires, not after which it expires.
--
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
Re: [PR] GUACAMOLE-1289: Duo upgrade to API v4 [guacamole-client]
Posted by "stcbus (via GitHub)" <gi...@apache.org>.
stcbus commented on PR #966:
URL: https://github.com/apache/guacamole-client/pull/966#issuecomment-2038577726
Thanks @aleitner ! In the interest of accuracy, I believe the application key is no longer used with v4, so that part can be removed from docker/config as well. Please correct, if wrong (I certainly don't pass it in my testing).
--
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