GUACAMOLE-990: Guarantee auth banning occurs before all other auth processing.

extensions/guacamole-auth-ban/src/main/java/org/apache/guacamole/auth/ban/BanningAuthenticationListener.java

@@ -21,41 +21,158 @@ package org.apache.guacamole.auth.ban;
 
 import org.apache.guacamole.auth.ban.status.AuthenticationFailureTracker;
 import org.apache.guacamole.GuacamoleException;
+import org.apache.guacamole.auth.ban.status.InMemoryAuthenticationFailureTracker;
+import org.apache.guacamole.auth.ban.status.NullAuthenticationFailureTracker;
+import org.apache.guacamole.environment.Environment;
+import org.apache.guacamole.environment.LocalEnvironment;
 import org.apache.guacamole.net.auth.credentials.GuacamoleInsufficientCredentialsException;
 import org.apache.guacamole.net.event.AuthenticationFailureEvent;
 import org.apache.guacamole.net.event.AuthenticationSuccessEvent;
 import org.apache.guacamole.net.event.listener.Listener;
+import org.apache.guacamole.net.event.AuthenticationRequestReceivedEvent;
+import org.apache.guacamole.properties.IntegerGuacamoleProperty;
+import org.apache.guacamole.properties.LongGuacamoleProperty;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
 
 /**
  * Listener implementation which automatically tracks authentication failures
- * such that further authentication attempts may be automatically blocked by
+ * such that further authentication attempts may be automatically blocked if
- * {@link BanningAuthenticationProvider} if they match configured criteria.
+ * they match configured criteria.
  */
 public class BanningAuthenticationListener implements Listener {
 
     /**
-     * Shared tracker of addresses that have repeatedly failed authentication.
+     * Logger for this class.
      */
-    private static AuthenticationFailureTracker tracker;
+    private static final Logger logger = LoggerFactory.getLogger(BanningAuthenticationListener.class);
 
     /**
-     * Assigns the shared tracker instance used by both the {@link BanningAuthenticationProvider}
+     * The maximum number of failed authentication attempts allowed before an
-     * and this listener. This function MUST be invoked with the tracker
+     * address is temporarily banned.
-     * created for BanningAuthenticationProvider as soon as possible (during
-     * construction of BanningAuthenticationProvider), or processing of
-     * received events will fail internally.
-     *
-     * @param tracker
-     *     The tracker instance to use for received authentication events.
      */
-    public static void setAuthenticationFailureTracker(AuthenticationFailureTracker tracker) {
+    private static final IntegerGuacamoleProperty MAX_ATTEMPTS = new IntegerGuacamoleProperty() {
-        BanningAuthenticationListener.tracker = tracker;
+
+        @Override
+        public String getName() {
+            return "ban-max-invalid-attempts";
+        }
+
+    };
+
+    /**
+     * The length of time that each address should be banned after reaching the
+     * maximum number of failed authentication attempts, in seconds.
+     */
+    private static final IntegerGuacamoleProperty IP_BAN_DURATION = new IntegerGuacamoleProperty() {
+
+        @Override
+        public String getName() {
+            return "ban-address-duration";
+        }
+
+    };
+
+    /**
+     * The maximum number of failed authentication attempts tracked at any
+     * given time. Once this number of addresses is exceeded, the oldest
+     * authentication attempts are rotated off on an LRU basis.
+     */
+    private static final LongGuacamoleProperty MAX_ADDRESSES = new LongGuacamoleProperty() {
+
+        @Override
+        public String getName() {
+            return "ban-max-addresses";
+        }
+
+    };
+
+    /**
+     * The default maximum number of failed authentication attempts allowed
+     * before an address is temporarily banned.
+     */
+    private static final int DEFAULT_MAX_ATTEMPTS = 5;
+
+    /**
+     * The default length of time that each address should be banned after
+     * reaching the maximum number of failed authentication attempts, in
+     * seconds.
+     */
+    private static final int DEFAULT_IP_BAN_DURATION = 300;
+
+    /**
+     * The maximum number of failed authentication attempts tracked at any
+     * given time. Once this number of addresses is exceeded, the oldest
+     * authentication attempts are rotated off on an LRU basis.
+     */
+    private static final long DEFAULT_MAX_ADDRESSES = 10485760;
+
+    /**
+     * Tracker of addresses that have repeatedly failed authentication.
+     */
+    private final AuthenticationFailureTracker tracker;
+
+    /**
+     * Creates a new BanningAuthenticationListener which automatically bans
+     * further authentication attempts from addresses that have repeatedly
+     * failed to authenticate. The ban duration and maximum number of failed
+     * attempts allowed before banning are configured within
+     * guacamole.properties.
+     *
+     * @throws GuacamoleException
+     *     If an error occurs parsing the configuration properties used by this
+     *     extension.
+     */
+    public BanningAuthenticationListener() throws GuacamoleException {
+
+        Environment environment = LocalEnvironment.getInstance();
+        int maxAttempts = environment.getProperty(MAX_ATTEMPTS, DEFAULT_MAX_ATTEMPTS);
+        int banDuration = environment.getProperty(IP_BAN_DURATION, DEFAULT_IP_BAN_DURATION);
+        long maxAddresses = environment.getProperty(MAX_ADDRESSES, DEFAULT_MAX_ADDRESSES);
+
+        // Configure auth failure tracking behavior and inform administrator of
+        // ultimate result
+        if (maxAttempts <= 0) {
+            this.tracker = new NullAuthenticationFailureTracker();
+            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) {
+            this.tracker = new NullAuthenticationFailureTracker();
+            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 if (maxAddresses <= 0) {
+            this.tracker = new NullAuthenticationFailureTracker();
+            logger.info("Maximum number of tracked addresses has been set to "
+                    + "{}. Automatic banning of brute-force authentication "
+                    + "attempts will be disabled.", maxAddresses);
+        }
+        else {
+            this.tracker = new InMemoryAuthenticationFailureTracker(maxAttempts, banDuration, maxAddresses);
+            logger.info("Addresses will be automatically banned for {} "
+                    + "seconds after {} failed authentication attempts. Up "
+                    + "to {} unique addresses will be tracked/banned at any "
+                    + "given time.", banDuration, maxAttempts, maxAddresses);
+        }
+
     }
 
     @Override
     public void handleEvent(Object event) throws GuacamoleException {
 
-        if (event instanceof AuthenticationFailureEvent) {
+        // Notify auth tracker of each request received BEFORE the request is
+        // processed ...
+        if (event instanceof AuthenticationRequestReceivedEvent) {
+            AuthenticationRequestReceivedEvent request = (AuthenticationRequestReceivedEvent) event;
+            tracker.notifyAuthenticationRequestReceived(request.getCredentials());
+        }
+
+        // ... as well as every explicit failure ...
+        else if (event instanceof AuthenticationFailureEvent) {
 
             AuthenticationFailureEvent failure = (AuthenticationFailureEvent) event;
 
@@ -72,6 +189,7 @@ public class BanningAuthenticationListener implements Listener {
 
         }
 
+        // ... and explicit success.
         else if (event instanceof AuthenticationSuccessEvent) {
             AuthenticationSuccessEvent success = (AuthenticationSuccessEvent) event;
             tracker.notifyAuthenticationSuccess(success.getCredentials());

extensions/guacamole-auth-ban/src/main/java/org/apache/guacamole/auth/ban/BanningAuthenticationProvider.java

@@ -1,182 +0,0 @@
-/*
- * 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 org.apache.guacamole.auth.ban.status.InMemoryAuthenticationFailureTracker;
-import org.apache.guacamole.auth.ban.status.AuthenticationFailureTracker;
-import org.apache.guacamole.GuacamoleException;
-import org.apache.guacamole.auth.ban.status.NullAuthenticationFailureTracker;
-import org.apache.guacamole.environment.Environment;
-import org.apache.guacamole.environment.LocalEnvironment;
-import org.apache.guacamole.net.auth.AbstractAuthenticationProvider;
-import org.apache.guacamole.net.auth.AuthenticatedUser;
-import org.apache.guacamole.net.auth.Credentials;
-import org.apache.guacamole.net.auth.UserContext;
-import org.apache.guacamole.properties.IntegerGuacamoleProperty;
-import org.apache.guacamole.properties.LongGuacamoleProperty;
-import org.slf4j.Logger;
-import org.slf4j.LoggerFactory;
-
-/**
- * AuthenticationProvider implementation that blocks further authentication
- * attempts that are related to past authentication failures flagged by
- * {@link BanningAuthenticationListener}.
- */
-public class BanningAuthenticationProvider extends AbstractAuthenticationProvider {
-
-    /**
-     * Logger for this class.
-     */
-    private static final Logger logger = LoggerFactory.getLogger(BanningAuthenticationProvider.class);
-
-    /**
-     * The maximum number of failed authentication attempts allowed before an
-     * address is temporarily banned.
-     */
-    private static final IntegerGuacamoleProperty MAX_ATTEMPTS = new IntegerGuacamoleProperty() {
-
-        @Override
-        public String getName() {
-            return "ban-max-invalid-attempts";
-        }
-
-    };
-
-    /**
-     * The length of time that each address should be banned after reaching the
-     * maximum number of failed authentication attempts, in seconds.
-     */
-    private static final IntegerGuacamoleProperty IP_BAN_DURATION = new IntegerGuacamoleProperty() {
-
-        @Override
-        public String getName() {
-            return "ban-address-duration";
-        }
-
-    };
-
-    /**
-     * The maximum number of failed authentication attempts tracked at any
-     * given time. Once this number of addresses is exceeded, the oldest
-     * authentication attempts are rotated off on an LRU basis.
-     */
-    private static final LongGuacamoleProperty MAX_ADDRESSES = new LongGuacamoleProperty() {
-
-        @Override
-        public String getName() {
-            return "ban-max-addresses";
-        }
-
-    };
-
-    /**
-     * The default maximum number of failed authentication attempts allowed
-     * before an address is temporarily banned.
-     */
-    private static final int DEFAULT_MAX_ATTEMPTS = 5;
-
-    /**
-     * The default length of time that each address should be banned after
-     * reaching the maximum number of failed authentication attempts, in
-     * seconds.
-     */
-    private static final int DEFAULT_IP_BAN_DURATION = 300;
-
-    /**
-     * The maximum number of failed authentication attempts tracked at any
-     * given time. Once this number of addresses is exceeded, the oldest
-     * authentication attempts are rotated off on an LRU basis.
-     */
-    private static final long DEFAULT_MAX_ADDRESSES = 10485760;
-
-    /**
-     * Shared tracker of addresses that have repeatedly failed authentication.
-     */
-    private final AuthenticationFailureTracker tracker;
-
-    /**
-     * Creates a new BanningAuthenticationProvider which automatically bans
-     * further authentication attempts from addresses that have repeatedly
-     * failed to authenticate. The ban duration and maximum number of failed
-     * attempts allowed before banning are configured within
-     * guacamole.properties.
-     *
-     * @throws GuacamoleException
-     *     If an error occurs parsing the configuration properties used by this
-     *     extension.
-     */
-    public BanningAuthenticationProvider() throws GuacamoleException {
-
-        Environment environment = LocalEnvironment.getInstance();
-        int maxAttempts = environment.getProperty(MAX_ATTEMPTS, DEFAULT_MAX_ATTEMPTS);
-        int banDuration = environment.getProperty(IP_BAN_DURATION, DEFAULT_IP_BAN_DURATION);
-        long maxAddresses = environment.getProperty(MAX_ADDRESSES, DEFAULT_MAX_ADDRESSES);
-
-        // Configure auth failure tracking behavior and inform administrator of
-        // ultimate result
-        if (maxAttempts <= 0) {
-            this.tracker = new NullAuthenticationFailureTracker();
-            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) {
-            this.tracker = new NullAuthenticationFailureTracker();
-            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 if (maxAddresses <= 0) {
-            this.tracker = new NullAuthenticationFailureTracker();
-            logger.info("Maximum number of tracked addresses has been set to "
-                    + "{}. Automatic banning of brute-force authentication "
-                    + "attempts will be disabled.", maxAddresses);
-        }
-        else {
-            this.tracker = new InMemoryAuthenticationFailureTracker(maxAttempts, banDuration, maxAddresses);
-            logger.info("Addresses will be automatically banned for {} "
-                    + "seconds after {} failed authentication attempts. Up "
-                    + "to {} unique addresses will be tracked/banned at any "
-                    + "given time.", banDuration, maxAttempts, maxAddresses);
-        }
-
-        BanningAuthenticationListener.setAuthenticationFailureTracker(tracker);
-
-    }
-
-    @Override
-    public String getIdentifier() {
-        return "ban";
-    }
-
-    @Override
-    public AuthenticatedUser authenticateUser(Credentials credentials) throws GuacamoleException {
-        tracker.notifyAuthenticationRequestReceived(credentials);
-        return null;
-    }
-
-    @Override
-    public UserContext getUserContext(AuthenticatedUser authenticatedUser) throws GuacamoleException {
-        tracker.notifyAuthenticationRequestReceived(authenticatedUser.getCredentials());
-        return null;
-    }
-
-}

extensions/guacamole-auth-ban/src/main/resources/guac-manifest.json

@@ -5,10 +5,6 @@
     "name"      : "Brute-force Authentication Detection/Prevention",
     "namespace" : "ban",
 
-    "authProviders" : [
-        "org.apache.guacamole.auth.ban.BanningAuthenticationProvider"
-    ],
-
     "listeners" : [
         "org.apache.guacamole.auth.ban.BanningAuthenticationListener"
     ],

guacamole-docker/bin/start.sh

@@ -1165,12 +1165,12 @@ set_optional_property "ban-address-duration" "$BAN_ADDRESS_DURATION"
 set_optional_property "ban-max-addresses" "$BAN_MAX_ADDRESSES"
 set_optional_property "ban-max-invalid-attempts" "$BAN_MAX_INVALID_ATTEMPTS"
 
-# Ensure guacamole-auth-ban always loads before other extensions unless
+# Always load guacamole-auth-ban extension (automatic banning can be disabled
-# explicitly overridden via naming or EXTENSION_PRIORITY (allowing other
+# through seting BAN_ADDRESS_DURATION to 0). As guacamole-auth-ban performs
-# extensions to attempt authentication before guacamole-auth-ban has a chance
+# its banning by handling a pre-authentication event, it is guaranteed to
-# to enforce any bans could allow credentials to continue to be guessed even
+# perform its checks before all other auth processing and load order does not
-# after the address has been blocked via timing attacks)
+# matter.
-ln -s /opt/guacamole/ban/guacamole-auth-*.jar "$GUACAMOLE_EXT/_guacamole-auth-ban.jar"
+ln -s /opt/guacamole/ban/guacamole-auth-*.jar "$GUACAMOLE_EXT"
 
 # Set logback level if specified
 if [ -n "$LOGBACK_LEVEL" ]; then

guacamole-ext/src/main/java/org/apache/guacamole/net/event/AuthenticationRequestReceivedEvent.java

@@ -0,0 +1,35 @@
+/*
+ * 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 is triggered whenever a user's credentials have been
+ * submitted for authentication, but that latest authentication request has not
+ * yet succeeded or failed. The credentials that were received for
+ * authentication are included within this event, and can be retrieved using
+ * {@link #getCredentials()}.
+ * <p>
+ * If a {@link org.apache.guacamole.net.event.listener.Listener} throws
+ * a GuacamoleException when handling an event of this type, the authentication
+ * request is entirely aborted as if it failed, and will be processed by any
+ * other listener or authentication provider.
+ */
+public interface AuthenticationRequestReceivedEvent extends CredentialEvent {
+}

guacamole/src/main/java/org/apache/guacamole/rest/auth/AuthenticationService.java

@@ -38,6 +38,7 @@ import org.apache.guacamole.net.auth.credentials.GuacamoleCredentialsException;
 import org.apache.guacamole.net.auth.credentials.GuacamoleInsufficientCredentialsException;
 import org.apache.guacamole.net.auth.credentials.GuacamoleInvalidCredentialsException;
 import org.apache.guacamole.net.event.AuthenticationFailureEvent;
+import org.apache.guacamole.net.event.AuthenticationRequestReceivedEvent;
 import org.apache.guacamole.net.event.AuthenticationSuccessEvent;
 import org.apache.guacamole.rest.event.ListenerService;
 import org.glassfish.jersey.server.ContainerRequest;
@@ -412,6 +413,9 @@ public class AuthenticationService {
     public String authenticate(Credentials credentials, String token)
             throws GuacamoleException {
 
+        // Fire pre-authentication event before ANY authn/authz occurs at all
+        listenerService.handleEvent((AuthenticationRequestReceivedEvent) () -> credentials);
+
         // Pull existing session if token provided
         GuacamoleSession existingSession;
         if (token != null)