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 2022/08/22 20:02:23 UTC
[GitHub] [guacamole-client] mike-jumper opened a new pull request, #758: GUACAMOLE-990: Add extension for automatically blocking brute-force auth attempts.
mike-jumper opened a new pull request, #758:
URL: https://github.com/apache/guacamole-client/pull/758
This change adds a new extension, "guacamole-auth-ban", which automatically tracks failed authentication attempts. After a specified limit has been reached, the IP address that is failing to authenticate is temporarily banned. This includes failures to provide valid MFA codes, etc., if Guacamole is configured to require them.
By default, addresses that repeatedly fail authentication are banned for 5 minutes (300 seconds) after 5 failed attempts, and these values can be overridden with the `ban-address-duration` and `ban-max-invalid-attempts` properties respectively.
The maximum number of addresses tracked is ~10 million by default (10485760), and this can be overridden with the `ban-max-addresses` property. It is intentionally not possible to remove the limit entirely, though it can be set as high as desired.
If too many authentication failures have occurred, the user failing to authenticate will see a message like:
> 
In addition to implementing the extension itself, this change involved:
* Modifying the firing of auth events such that they are always fired only after authentication has entirely succeeded or entirely failed.
* Modifying handling of auth failures such that past auth tokens are removed from client-side storage after they have been determined to be invalid (they will otherwise be pointlessly resubmitted and will count toward auth failures).
--
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 diff in pull request #758: GUACAMOLE-990: Add extension for automatically blocking brute-force auth attempts.
Posted by GitBox <gi...@apache.org>.
mike-jumper commented on code in PR #758:
URL: https://github.com/apache/guacamole-client/pull/758#discussion_r951966652
##########
guacamole/src/main/java/org/apache/guacamole/rest/auth/AuthenticationService.java:
##########
@@ -462,25 +419,72 @@ public String authenticate(Credentials credentials, String token)
else
existingSession = null;
- // Get up-to-date AuthenticatedUser and associated UserContexts
- AuthenticatedUser authenticatedUser = getAuthenticatedUser(existingSession, credentials);
- List<DecoratedUserContext> userContexts = getUserContexts(existingSession, authenticatedUser, credentials);
-
- // Update existing session, if it exists
+ AuthenticatedUser authenticatedUser;
String authToken;
- if (existingSession != null) {
- authToken = token;
- existingSession.setAuthenticatedUser(authenticatedUser);
- existingSession.setUserContexts(userContexts);
+
+ try {
+
+ // Get up-to-date AuthenticatedUser and associated UserContexts
+ authenticatedUser = getAuthenticatedUser(existingSession, credentials);
+ List<DecoratedUserContext> userContexts = getUserContexts(existingSession, authenticatedUser, credentials);
+
+ // Update existing session, if it exists
+ if (existingSession != null) {
+ authToken = token;
+ existingSession.setAuthenticatedUser(authenticatedUser);
+ existingSession.setUserContexts(userContexts);
+ }
+
+ // If no existing session, generate a new token/session pair
+ else {
+ authToken = authTokenGenerator.getToken();
+ tokenSessionMap.put(authToken, new GuacamoleSession(environment, authenticatedUser, userContexts));
+ logger.debug("Login was successful for user \"{}\".", authenticatedUser.getIdentifier());
+ }
+
+ // Report authentication success
+ try {
+ listenerService.handleEvent(new AuthenticationSuccessEvent(authenticatedUser));
+ }
+ catch (GuacamoleException e) {
+ throw new GuacamoleAuthenticationProcessException("User "
+ + "authentication aborted by event listener.", null, e);
+ }
+
}
- // If no existing session, generate a new token/session pair
- else {
- authToken = authTokenGenerator.getToken();
- tokenSessionMap.put(authToken, new GuacamoleSession(environment, authenticatedUser, userContexts));
- logger.debug("Login was successful for user \"{}\".", authenticatedUser.getIdentifier());
+ // Log and rethrow any authentication errors
+ catch (GuacamoleAuthenticationProcessException e) {
+
+ // Get request and username for sake of logging
+ HttpServletRequest request = credentials.getRequest();
+ String username = credentials.getUsername();
+
+ listenerService.handleEvent(new AuthenticationFailureEvent(credentials,
+ e.getAuthenticationProvider(), e.getCause()));
+
+ // Log authentication failures with associated usernames
+ if (username != null) {
+ if (logger.isWarnEnabled())
+ logger.warn("Authentication attempt from {} for user \"{}\" failed.",
+ getLoggableAddress(request), username);
+ }
+
+ // Log anonymous authentication failures
+ else if (logger.isDebugEnabled())
+ logger.debug("Anonymous authentication attempt from {} failed.",
+ getLoggableAddress(request));
+
+ // Rethrow exception
+ throw e.getCauseAsGuacamoleException();
+
}
+ if (logger.isInfoEnabled())
Review Comment:
IIRC, these checks were added because of the call to `getLoggableAddress()`, which performs string validation with a regex and is thus relatively heavy:
https://github.com/apache/guacamole-client/blob/843add93a5f2dc24e16b9370f8b95ad55eafe5bf/guacamole/src/main/java/org/apache/guacamole/rest/auth/AuthenticationService.java#L146-L156
If we took out these checks, `getLoggableAddress()` would be called in every case because of how its return value is passed, even if the relevant log level is disabled.
--
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] jmuehlner merged pull request #758: GUACAMOLE-990: Add extension for automatically blocking brute-force auth attempts.
Posted by GitBox <gi...@apache.org>.
jmuehlner merged PR #758:
URL: https://github.com/apache/guacamole-client/pull/758
--
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] jmuehlner commented on a diff in pull request #758: GUACAMOLE-990: Add extension for automatically blocking brute-force auth attempts.
Posted by GitBox <gi...@apache.org>.
jmuehlner commented on code in PR #758:
URL: https://github.com/apache/guacamole-client/pull/758#discussion_r951943046
##########
extensions/guacamole-auth-ban/src/main/java/org/apache/guacamole/auth/ban/AuthenticationFailureTracker.java:
##########
@@ -0,0 +1,290 @@
+/*
+ * 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.ban;
+
+import com.github.benmanes.caffeine.cache.Cache;
+import com.github.benmanes.caffeine.cache.Caffeine;
+import javax.servlet.http.HttpServletRequest;
+import org.apache.guacamole.GuacamoleException;
+import org.apache.guacamole.GuacamoleServerException;
+import org.apache.guacamole.language.TranslatableGuacamoleClientTooManyException;
+import org.apache.guacamole.net.auth.Credentials;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * Provides automated tracking and blocking of IP addresses that repeatedly
+ * fail authentication.
+ */
+public class AuthenticationFailureTracker {
+
+ /**
+ * Logger for this class.
+ */
+ private static final Logger logger = LoggerFactory.getLogger(AuthenticationFailureTracker.class);
+
+ /**
+ * All authentication failures currently being tracked, stored by the
+ * associated IP address.
+ */
+ private final Cache<String, AuthenticationFailureStatus> failures;
+
+ /**
+ * The maximum number of failed authentication attempts allowed before an
+ * address is temporarily banned.
+ */
+ private final int maxAttempts;
+
+ /**
+ * The length of time that each address should be banned after reaching the
+ * maximum number of failed authentication attempts, in seconds.
+ */
+ private final int banDuration;
+
+ /**
+ * Creates a new AuthenticationFailureTracker that automatically blocks
+ * authentication attempts based on the provided blocking criteria.
+ *
+ * @param maxAttempts
+ * The maximum number of failed authentication attempts allowed before
+ * an address is temporarily banned.
+ *
+ * @param banDuration
+ * The length of time that each address should be banned after reaching
+ * the maximum number of failed authentication attempts, in seconds.
+ *
+ * @param maxAddresses
+ * The maximum number of unique IP addresses that should be tracked
+ * before discarding older tracked failures.
+ */
+ public AuthenticationFailureTracker(int maxAttempts, int banDuration,
+ long maxAddresses) {
+
+ this.maxAttempts = maxAttempts;
+ this.banDuration = banDuration;
+
+ // Inform administrator of configured behavior
+ if (maxAttempts <= 0) {
+ logger.info("Maximum failed authentication attempts has been set "
+ + "to {}. Automatic banning of brute-force authentication "
+ + "attempts will be disabled.", maxAttempts);
+ }
+ else if (banDuration <= 0) {
+ logger.info("Ban duration for addresses that repeatedly fail "
+ + "authentication has been set to {}. Automatic banning "
+ + "of brute-force authentication attempts will be "
+ + "disabled.", banDuration);
+ }
+ else {
+ logger.info("Addresses will be automatically banned for {} "
+ + "seconds after {} failed authentication attempts.",
+ banDuration, maxAttempts);
+ }
+
+ // Limit maximum number of tracked addresses to configured upper bound
+ this.failures = Caffeine.newBuilder()
+ .maximumSize(maxAddresses)
+ .build();
+
+ logger.info("Up to {} unique addresses will be tracked/banned at any "
Review Comment:
Actually, looks like tracking is also disabled, per line 191, so maybe best to just get rid of the whole logging 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
[GitHub] [guacamole-client] jmuehlner commented on a diff in pull request #758: GUACAMOLE-990: Add extension for automatically blocking brute-force auth attempts.
Posted by GitBox <gi...@apache.org>.
jmuehlner commented on code in PR #758:
URL: https://github.com/apache/guacamole-client/pull/758#discussion_r951984635
##########
guacamole/src/main/java/org/apache/guacamole/rest/auth/AuthenticationService.java:
##########
@@ -392,7 +333,15 @@ private List<DecoratedUserContext> getUserContexts(GuacamoleSession existingSess
// Update existing UserContext
AuthenticationProvider authProvider = oldUserContext.getAuthenticationProvider();
- UserContext updatedUserContext = authProvider.updateUserContext(oldUserContext, authenticatedUser, credentials);
+ UserContext updatedUserContext;
+ try {
+ updatedUserContext = authProvider.updateUserContext(oldUserContext, authenticatedUser, credentials);
+ }
+ catch (GuacamoleException | RuntimeException | Error e) {
Review Comment:
Makes sense.
--
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] jmuehlner commented on a diff in pull request #758: GUACAMOLE-990: Add extension for automatically blocking brute-force auth attempts.
Posted by GitBox <gi...@apache.org>.
jmuehlner commented on code in PR #758:
URL: https://github.com/apache/guacamole-client/pull/758#discussion_r951984553
##########
guacamole-ext/src/main/java/org/apache/guacamole/net/event/FailureEvent.java:
##########
@@ -0,0 +1,39 @@
+/*
+ * 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.net.event;
+
+/**
+ * An event which represents failure of an operation, where that failure may
+ * be associated with a particular Throwable.
+ */
+public interface FailureEvent {
Review Comment:
Makes sense
--
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] jmuehlner commented on a diff in pull request #758: GUACAMOLE-990: Add extension for automatically blocking brute-force auth attempts.
Posted by GitBox <gi...@apache.org>.
jmuehlner commented on code in PR #758:
URL: https://github.com/apache/guacamole-client/pull/758#discussion_r951944531
##########
guacamole/src/main/frontend/src/app/auth/service/authenticationService.js:
##########
@@ -160,6 +160,8 @@ angular.module('auth').factory('authenticationService', ['$injector',
url: 'api/tokens',
headers: {
'Content-Type': 'application/x-www-form-urlencoded'
+ // FIXME: Clear current token if invalid, send token as header
Review Comment:
Is this still in need of fixing?
--
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 diff in pull request #758: GUACAMOLE-990: Add extension for automatically blocking brute-force auth attempts.
Posted by GitBox <gi...@apache.org>.
mike-jumper commented on code in PR #758:
URL: https://github.com/apache/guacamole-client/pull/758#discussion_r951960987
##########
extensions/guacamole-auth-ban/src/main/java/org/apache/guacamole/auth/ban/AuthenticationFailureTracker.java:
##########
@@ -0,0 +1,290 @@
+/*
+ * 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.ban;
+
+import com.github.benmanes.caffeine.cache.Cache;
+import com.github.benmanes.caffeine.cache.Caffeine;
+import javax.servlet.http.HttpServletRequest;
+import org.apache.guacamole.GuacamoleException;
+import org.apache.guacamole.GuacamoleServerException;
+import org.apache.guacamole.language.TranslatableGuacamoleClientTooManyException;
+import org.apache.guacamole.net.auth.Credentials;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * Provides automated tracking and blocking of IP addresses that repeatedly
+ * fail authentication.
+ */
+public class AuthenticationFailureTracker {
+
+ /**
+ * Logger for this class.
+ */
+ private static final Logger logger = LoggerFactory.getLogger(AuthenticationFailureTracker.class);
+
+ /**
+ * All authentication failures currently being tracked, stored by the
+ * associated IP address.
+ */
+ private final Cache<String, AuthenticationFailureStatus> failures;
+
+ /**
+ * The maximum number of failed authentication attempts allowed before an
+ * address is temporarily banned.
+ */
+ private final int maxAttempts;
+
+ /**
+ * The length of time that each address should be banned after reaching the
+ * maximum number of failed authentication attempts, in seconds.
+ */
+ private final int banDuration;
+
+ /**
+ * Creates a new AuthenticationFailureTracker that automatically blocks
+ * authentication attempts based on the provided blocking criteria.
+ *
+ * @param maxAttempts
+ * The maximum number of failed authentication attempts allowed before
+ * an address is temporarily banned.
+ *
+ * @param banDuration
+ * The length of time that each address should be banned after reaching
+ * the maximum number of failed authentication attempts, in seconds.
+ *
+ * @param maxAddresses
+ * The maximum number of unique IP addresses that should be tracked
+ * before discarding older tracked failures.
+ */
+ public AuthenticationFailureTracker(int maxAttempts, int banDuration,
+ long maxAddresses) {
+
+ this.maxAttempts = maxAttempts;
+ this.banDuration = banDuration;
+
+ // Inform administrator of configured behavior
+ if (maxAttempts <= 0) {
+ logger.info("Maximum failed authentication attempts has been set "
+ + "to {}. Automatic banning of brute-force authentication "
+ + "attempts will be disabled.", maxAttempts);
+ }
+ else if (banDuration <= 0) {
+ logger.info("Ban duration for addresses that repeatedly fail "
+ + "authentication has been set to {}. Automatic banning "
+ + "of brute-force authentication attempts will be "
+ + "disabled.", banDuration);
+ }
+ else {
+ logger.info("Addresses will be automatically banned for {} "
+ + "seconds after {} failed authentication attempts.",
+ banDuration, maxAttempts);
+ }
+
+ // Limit maximum number of tracked addresses to configured upper bound
+ this.failures = Caffeine.newBuilder()
+ .maximumSize(maxAddresses)
+ .build();
+
+ logger.info("Up to {} unique addresses will be tracked/banned at any "
Review Comment:
OK - I've refactored a bit, extracting a base interface and moving this logic elsewhere. The log message should be more sensible now, with this one not being present at all unless things are enabled.
##########
guacamole/src/main/frontend/src/app/auth/service/authenticationService.js:
##########
@@ -160,6 +160,8 @@ angular.module('auth').factory('authenticationService', ['$injector',
url: 'api/tokens',
headers: {
'Content-Type': 'application/x-www-form-urlencoded'
+ // FIXME: Clear current token if invalid, send token as header
Review Comment:
Removed 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] jmuehlner commented on a diff in pull request #758: GUACAMOLE-990: Add extension for automatically blocking brute-force auth attempts.
Posted by GitBox <gi...@apache.org>.
jmuehlner commented on code in PR #758:
URL: https://github.com/apache/guacamole-client/pull/758#discussion_r951955594
##########
guacamole-ext/src/main/java/org/apache/guacamole/net/event/FailureEvent.java:
##########
@@ -0,0 +1,39 @@
+/*
+ * 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.net.event;
+
+/**
+ * An event which represents failure of an operation, where that failure may
+ * be associated with a particular Throwable.
+ */
+public interface FailureEvent {
Review Comment:
Are there any other use cases for this interface? Feels a bit funny to create this and then only ever use the concrete implementation.
--
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 pull request #758: GUACAMOLE-990: Add extension for automatically blocking brute-force auth attempts.
Posted by GitBox <gi...@apache.org>.
mike-jumper commented on PR #758:
URL: https://github.com/apache/guacamole-client/pull/758#issuecomment-1222970355
**NOTE:** This extension works by aborting authentication early by throwing a `GuacamoleClientTooManyException`, which immediately aborts via the following code path:
https://github.com/apache/guacamole-client/blob/fe56df73fbbe640ed276bac823bcfb2f296ac493/guacamole/src/main/java/org/apache/guacamole/rest/auth/AuthenticationService.java#L208-L211
This is naturally dependent on the order that extensions are loaded, hence naming things such that it's loaded first within the Docker image:
https://github.com/apache/guacamole-client/blob/fe56df73fbbe640ed276bac823bcfb2f296ac493/guacamole-docker/bin/start.sh#L1168-L1173
**Loading the extension before all other auth extensions is necessary for correct behavior with respect to timing**, and we'll have to document this in the manual.
If the extension is installed but _not_ loaded first, then extensions that load earlier will be given a chance to authenticate the user before guacamole-auth-ban can abort the auth process. Even though repeated auth attempts will still be blocked, the amount of time taken until that block occurs might vary by whether the credentials provided were valid according to those other extensions, and that variance in timing might allow an attacker to determine whether their guess is correct even though full auth is temporarily blocked.
--
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] jmuehlner commented on a diff in pull request #758: GUACAMOLE-990: Add extension for automatically blocking brute-force auth attempts.
Posted by GitBox <gi...@apache.org>.
jmuehlner commented on code in PR #758:
URL: https://github.com/apache/guacamole-client/pull/758#discussion_r951947278
##########
guacamole/src/main/java/org/apache/guacamole/rest/auth/AuthenticationService.java:
##########
@@ -392,7 +333,15 @@ private List<DecoratedUserContext> getUserContexts(GuacamoleSession existingSess
// Update existing UserContext
AuthenticationProvider authProvider = oldUserContext.getAuthenticationProvider();
- UserContext updatedUserContext = authProvider.updateUserContext(oldUserContext, authenticatedUser, credentials);
+ UserContext updatedUserContext;
+ try {
+ updatedUserContext = authProvider.updateUserContext(oldUserContext, authenticatedUser, credentials);
+ }
+ catch (GuacamoleException | RuntimeException | Error e) {
Review Comment:
This feels a little overly broad to be catching `RuntimeException`s and `Error`s. Why catch those now?
--
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] jmuehlner commented on a diff in pull request #758: GUACAMOLE-990: Add extension for automatically blocking brute-force auth attempts.
Posted by GitBox <gi...@apache.org>.
jmuehlner commented on code in PR #758:
URL: https://github.com/apache/guacamole-client/pull/758#discussion_r951950205
##########
guacamole/src/main/java/org/apache/guacamole/rest/auth/AuthenticationService.java:
##########
@@ -462,25 +419,72 @@ public String authenticate(Credentials credentials, String token)
else
existingSession = null;
- // Get up-to-date AuthenticatedUser and associated UserContexts
- AuthenticatedUser authenticatedUser = getAuthenticatedUser(existingSession, credentials);
- List<DecoratedUserContext> userContexts = getUserContexts(existingSession, authenticatedUser, credentials);
-
- // Update existing session, if it exists
+ AuthenticatedUser authenticatedUser;
String authToken;
- if (existingSession != null) {
- authToken = token;
- existingSession.setAuthenticatedUser(authenticatedUser);
- existingSession.setUserContexts(userContexts);
+
+ try {
+
+ // Get up-to-date AuthenticatedUser and associated UserContexts
+ authenticatedUser = getAuthenticatedUser(existingSession, credentials);
+ List<DecoratedUserContext> userContexts = getUserContexts(existingSession, authenticatedUser, credentials);
+
+ // Update existing session, if it exists
+ if (existingSession != null) {
+ authToken = token;
+ existingSession.setAuthenticatedUser(authenticatedUser);
+ existingSession.setUserContexts(userContexts);
+ }
+
+ // If no existing session, generate a new token/session pair
+ else {
+ authToken = authTokenGenerator.getToken();
+ tokenSessionMap.put(authToken, new GuacamoleSession(environment, authenticatedUser, userContexts));
+ logger.debug("Login was successful for user \"{}\".", authenticatedUser.getIdentifier());
+ }
+
+ // Report authentication success
+ try {
+ listenerService.handleEvent(new AuthenticationSuccessEvent(authenticatedUser));
+ }
+ catch (GuacamoleException e) {
+ throw new GuacamoleAuthenticationProcessException("User "
+ + "authentication aborted by event listener.", null, e);
+ }
+
}
- // If no existing session, generate a new token/session pair
- else {
- authToken = authTokenGenerator.getToken();
- tokenSessionMap.put(authToken, new GuacamoleSession(environment, authenticatedUser, userContexts));
- logger.debug("Login was successful for user \"{}\".", authenticatedUser.getIdentifier());
+ // Log and rethrow any authentication errors
+ catch (GuacamoleAuthenticationProcessException e) {
+
+ // Get request and username for sake of logging
+ HttpServletRequest request = credentials.getRequest();
+ String username = credentials.getUsername();
+
+ listenerService.handleEvent(new AuthenticationFailureEvent(credentials,
+ e.getAuthenticationProvider(), e.getCause()));
+
+ // Log authentication failures with associated usernames
+ if (username != null) {
+ if (logger.isWarnEnabled())
+ logger.warn("Authentication attempt from {} for user \"{}\" failed.",
+ getLoggableAddress(request), username);
+ }
+
+ // Log anonymous authentication failures
+ else if (logger.isDebugEnabled())
+ logger.debug("Anonymous authentication attempt from {} failed.",
+ getLoggableAddress(request));
+
+ // Rethrow exception
+ throw e.getCauseAsGuacamoleException();
+
}
+ if (logger.isInfoEnabled())
Review Comment:
Is `isInfoEnabled()` really needed here? The slf4j folks appear to recommend skipping this explicit check as long as we use the parameterized methods (which we do).
https://www.slf4j.org/faq.html#logging_performance
--
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] jmuehlner commented on a diff in pull request #758: GUACAMOLE-990: Add extension for automatically blocking brute-force auth attempts.
Posted by GitBox <gi...@apache.org>.
jmuehlner commented on code in PR #758:
URL: https://github.com/apache/guacamole-client/pull/758#discussion_r951950878
##########
guacamole/src/main/java/org/apache/guacamole/rest/auth/AuthenticationService.java:
##########
@@ -462,25 +419,72 @@ public String authenticate(Credentials credentials, String token)
else
existingSession = null;
- // Get up-to-date AuthenticatedUser and associated UserContexts
- AuthenticatedUser authenticatedUser = getAuthenticatedUser(existingSession, credentials);
- List<DecoratedUserContext> userContexts = getUserContexts(existingSession, authenticatedUser, credentials);
-
- // Update existing session, if it exists
+ AuthenticatedUser authenticatedUser;
String authToken;
- if (existingSession != null) {
- authToken = token;
- existingSession.setAuthenticatedUser(authenticatedUser);
- existingSession.setUserContexts(userContexts);
+
+ try {
+
+ // Get up-to-date AuthenticatedUser and associated UserContexts
+ authenticatedUser = getAuthenticatedUser(existingSession, credentials);
+ List<DecoratedUserContext> userContexts = getUserContexts(existingSession, authenticatedUser, credentials);
+
+ // Update existing session, if it exists
+ if (existingSession != null) {
+ authToken = token;
+ existingSession.setAuthenticatedUser(authenticatedUser);
+ existingSession.setUserContexts(userContexts);
+ }
+
+ // If no existing session, generate a new token/session pair
+ else {
+ authToken = authTokenGenerator.getToken();
+ tokenSessionMap.put(authToken, new GuacamoleSession(environment, authenticatedUser, userContexts));
+ logger.debug("Login was successful for user \"{}\".", authenticatedUser.getIdentifier());
+ }
+
+ // Report authentication success
+ try {
+ listenerService.handleEvent(new AuthenticationSuccessEvent(authenticatedUser));
+ }
+ catch (GuacamoleException e) {
+ throw new GuacamoleAuthenticationProcessException("User "
+ + "authentication aborted by event listener.", null, e);
+ }
+
}
- // If no existing session, generate a new token/session pair
- else {
- authToken = authTokenGenerator.getToken();
- tokenSessionMap.put(authToken, new GuacamoleSession(environment, authenticatedUser, userContexts));
- logger.debug("Login was successful for user \"{}\".", authenticatedUser.getIdentifier());
+ // Log and rethrow any authentication errors
+ catch (GuacamoleAuthenticationProcessException e) {
+
+ // Get request and username for sake of logging
+ HttpServletRequest request = credentials.getRequest();
+ String username = credentials.getUsername();
+
+ listenerService.handleEvent(new AuthenticationFailureEvent(credentials,
+ e.getAuthenticationProvider(), e.getCause()));
+
+ // Log authentication failures with associated usernames
+ if (username != null) {
+ if (logger.isWarnEnabled())
+ logger.warn("Authentication attempt from {} for user \"{}\" failed.",
+ getLoggableAddress(request), username);
+ }
+
+ // Log anonymous authentication failures
+ else if (logger.isDebugEnabled())
+ logger.debug("Anonymous authentication attempt from {} failed.",
+ getLoggableAddress(request));
+
+ // Rethrow exception
+ throw e.getCauseAsGuacamoleException();
+
}
+ if (logger.isInfoEnabled())
Review Comment:
Ah nevermind, I see the call to `getLoggableAddress`.
--
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 diff in pull request #758: GUACAMOLE-990: Add extension for automatically blocking brute-force auth attempts.
Posted by GitBox <gi...@apache.org>.
mike-jumper commented on code in PR #758:
URL: https://github.com/apache/guacamole-client/pull/758#discussion_r951959208
##########
guacamole/src/main/frontend/src/app/auth/service/authenticationService.js:
##########
@@ -160,6 +160,8 @@ angular.module('auth').factory('authenticationService', ['$injector',
url: 'api/tokens',
headers: {
'Content-Type': 'application/x-www-form-urlencoded'
+ // FIXME: Clear current token if invalid, send token as header
Review Comment:
Oops - nope, I addressed that. Will remove.
--
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 diff in pull request #758: GUACAMOLE-990: Add extension for automatically blocking brute-force auth attempts.
Posted by GitBox <gi...@apache.org>.
mike-jumper commented on code in PR #758:
URL: https://github.com/apache/guacamole-client/pull/758#discussion_r951975947
##########
guacamole/src/main/java/org/apache/guacamole/rest/auth/AuthenticationService.java:
##########
@@ -392,7 +333,15 @@ private List<DecoratedUserContext> getUserContexts(GuacamoleSession existingSess
// Update existing UserContext
AuthenticationProvider authProvider = oldUserContext.getAuthenticationProvider();
- UserContext updatedUserContext = authProvider.updateUserContext(oldUserContext, authenticatedUser, credentials);
+ UserContext updatedUserContext;
+ try {
+ updatedUserContext = authProvider.updateUserContext(oldUserContext, authenticatedUser, credentials);
+ }
+ catch (GuacamoleException | RuntimeException | Error e) {
Review Comment:
At the authorization stage, any error whatsoever from any extension will abort the authorization process and fail overall authentication. If this `catch` were not broad enough to also include unchecked exceptions/errors, then those errors would not be listenable via `Listener` and `FailureEvent`.
--
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 diff in pull request #758: GUACAMOLE-990: Add extension for automatically blocking brute-force auth attempts.
Posted by GitBox <gi...@apache.org>.
mike-jumper commented on code in PR #758:
URL: https://github.com/apache/guacamole-client/pull/758#discussion_r951974382
##########
guacamole-ext/src/main/java/org/apache/guacamole/net/event/FailureEvent.java:
##########
@@ -0,0 +1,39 @@
+/*
+ * 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.net.event;
+
+/**
+ * An event which represents failure of an operation, where that failure may
+ * be associated with a particular Throwable.
+ */
+public interface FailureEvent {
Review Comment:
The pattern that's been applied for events received by `Listener` has been to abstract away just about every aspect of the event into a separate interface, to ensure that `Listener` implementations can test for and handle received events based on any common aspect they might be interested in:
* [`CredentialEvent`](https://guacamole.apache.org/doc/guacamole-ext/org/apache/guacamole/net/event/CredentialEvent.html)
* [`TunnelEvent`](https://guacamole.apache.org/doc/guacamole-ext/org/apache/guacamole/net/event/TunnelEvent.html)
* [`UserEvent`](https://guacamole.apache.org/doc/guacamole-ext/org/apache/guacamole/net/event/UserEvent.html)
`AuthenticationProviderEvent` and `FailureEvent` were added here with this pattern in mind. Implementations of `Listener` that are interested in failures of any kind can simply listen for `FailureEvent` and rely on receiving those events, even if new failure-type events are introduced in the future, and the number of `instanceof` checks can be kept minimal.
--
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] jmuehlner commented on a diff in pull request #758: GUACAMOLE-990: Add extension for automatically blocking brute-force auth attempts.
Posted by GitBox <gi...@apache.org>.
jmuehlner commented on code in PR #758:
URL: https://github.com/apache/guacamole-client/pull/758#discussion_r951939381
##########
extensions/guacamole-auth-ban/src/main/java/org/apache/guacamole/auth/ban/AuthenticationFailureTracker.java:
##########
@@ -0,0 +1,290 @@
+/*
+ * 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.ban;
+
+import com.github.benmanes.caffeine.cache.Cache;
+import com.github.benmanes.caffeine.cache.Caffeine;
+import javax.servlet.http.HttpServletRequest;
+import org.apache.guacamole.GuacamoleException;
+import org.apache.guacamole.GuacamoleServerException;
+import org.apache.guacamole.language.TranslatableGuacamoleClientTooManyException;
+import org.apache.guacamole.net.auth.Credentials;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * Provides automated tracking and blocking of IP addresses that repeatedly
+ * fail authentication.
+ */
+public class AuthenticationFailureTracker {
+
+ /**
+ * Logger for this class.
+ */
+ private static final Logger logger = LoggerFactory.getLogger(AuthenticationFailureTracker.class);
+
+ /**
+ * All authentication failures currently being tracked, stored by the
+ * associated IP address.
+ */
+ private final Cache<String, AuthenticationFailureStatus> failures;
+
+ /**
+ * The maximum number of failed authentication attempts allowed before an
+ * address is temporarily banned.
+ */
+ private final int maxAttempts;
+
+ /**
+ * The length of time that each address should be banned after reaching the
+ * maximum number of failed authentication attempts, in seconds.
+ */
+ private final int banDuration;
+
+ /**
+ * Creates a new AuthenticationFailureTracker that automatically blocks
+ * authentication attempts based on the provided blocking criteria.
+ *
+ * @param maxAttempts
+ * The maximum number of failed authentication attempts allowed before
+ * an address is temporarily banned.
+ *
+ * @param banDuration
+ * The length of time that each address should be banned after reaching
+ * the maximum number of failed authentication attempts, in seconds.
+ *
+ * @param maxAddresses
+ * The maximum number of unique IP addresses that should be tracked
+ * before discarding older tracked failures.
+ */
+ public AuthenticationFailureTracker(int maxAttempts, int banDuration,
+ long maxAddresses) {
+
+ this.maxAttempts = maxAttempts;
+ this.banDuration = banDuration;
+
+ // Inform administrator of configured behavior
+ if (maxAttempts <= 0) {
+ logger.info("Maximum failed authentication attempts has been set "
+ + "to {}. Automatic banning of brute-force authentication "
+ + "attempts will be disabled.", maxAttempts);
+ }
+ else if (banDuration <= 0) {
+ logger.info("Ban duration for addresses that repeatedly fail "
+ + "authentication has been set to {}. Automatic banning "
+ + "of brute-force authentication attempts will be "
+ + "disabled.", banDuration);
+ }
+ else {
+ logger.info("Addresses will be automatically banned for {} "
+ + "seconds after {} failed authentication attempts.",
+ banDuration, maxAttempts);
+ }
+
+ // Limit maximum number of tracked addresses to configured upper bound
+ this.failures = Caffeine.newBuilder()
+ .maximumSize(maxAddresses)
+ .build();
+
+ logger.info("Up to {} unique addresses will be tracked/banned at any "
Review Comment:
This sort of still sounds like it's going to ban addresses even if disabled by `maxAttempts` or `banDuration` being <= 0. Consider cutting out the "/banned" if that functionality is disabled?
--
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 diff in pull request #758: GUACAMOLE-990: Add extension for automatically blocking brute-force auth attempts.
Posted by GitBox <gi...@apache.org>.
mike-jumper commented on code in PR #758:
URL: https://github.com/apache/guacamole-client/pull/758#discussion_r951942553
##########
extensions/guacamole-auth-ban/src/main/java/org/apache/guacamole/auth/ban/AuthenticationFailureTracker.java:
##########
@@ -0,0 +1,290 @@
+/*
+ * 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.ban;
+
+import com.github.benmanes.caffeine.cache.Cache;
+import com.github.benmanes.caffeine.cache.Caffeine;
+import javax.servlet.http.HttpServletRequest;
+import org.apache.guacamole.GuacamoleException;
+import org.apache.guacamole.GuacamoleServerException;
+import org.apache.guacamole.language.TranslatableGuacamoleClientTooManyException;
+import org.apache.guacamole.net.auth.Credentials;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * Provides automated tracking and blocking of IP addresses that repeatedly
+ * fail authentication.
+ */
+public class AuthenticationFailureTracker {
+
+ /**
+ * Logger for this class.
+ */
+ private static final Logger logger = LoggerFactory.getLogger(AuthenticationFailureTracker.class);
+
+ /**
+ * All authentication failures currently being tracked, stored by the
+ * associated IP address.
+ */
+ private final Cache<String, AuthenticationFailureStatus> failures;
+
+ /**
+ * The maximum number of failed authentication attempts allowed before an
+ * address is temporarily banned.
+ */
+ private final int maxAttempts;
+
+ /**
+ * The length of time that each address should be banned after reaching the
+ * maximum number of failed authentication attempts, in seconds.
+ */
+ private final int banDuration;
+
+ /**
+ * Creates a new AuthenticationFailureTracker that automatically blocks
+ * authentication attempts based on the provided blocking criteria.
+ *
+ * @param maxAttempts
+ * The maximum number of failed authentication attempts allowed before
+ * an address is temporarily banned.
+ *
+ * @param banDuration
+ * The length of time that each address should be banned after reaching
+ * the maximum number of failed authentication attempts, in seconds.
+ *
+ * @param maxAddresses
+ * The maximum number of unique IP addresses that should be tracked
+ * before discarding older tracked failures.
+ */
+ public AuthenticationFailureTracker(int maxAttempts, int banDuration,
+ long maxAddresses) {
+
+ this.maxAttempts = maxAttempts;
+ this.banDuration = banDuration;
+
+ // Inform administrator of configured behavior
+ if (maxAttempts <= 0) {
+ logger.info("Maximum failed authentication attempts has been set "
+ + "to {}. Automatic banning of brute-force authentication "
+ + "attempts will be disabled.", maxAttempts);
+ }
+ else if (banDuration <= 0) {
+ logger.info("Ban duration for addresses that repeatedly fail "
+ + "authentication has been set to {}. Automatic banning "
+ + "of brute-force authentication attempts will be "
+ + "disabled.", banDuration);
+ }
+ else {
+ logger.info("Addresses will be automatically banned for {} "
+ + "seconds after {} failed authentication attempts.",
+ banDuration, maxAttempts);
+ }
+
+ // Limit maximum number of tracked addresses to configured upper bound
+ this.failures = Caffeine.newBuilder()
+ .maximumSize(maxAddresses)
+ .build();
+
+ logger.info("Up to {} unique addresses will be tracked/banned at any "
Review Comment:
Sure.
--
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