GUACAMOLE-990: Merge change ensuring banning occurs before other auth.

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

@@ -21,41 +21,158 @@
 
 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
- * {@link BanningAuthenticationProvider} if they match configured criteria.
+ * such that further authentication attempts may be automatically blocked if
+ * 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}
-     * and this listener. This function MUST be invoked with the tracker
-     * created for BanningAuthenticationProvider as soon as possible (during
-     * construction of BanningAuthenticationProvider), or processing of
-     * received events will fail internally.
+     * 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;
+
+    /**
+     * 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.
      *
-     * @param tracker
-     *     The tracker instance to use for received authentication events.
+     * @throws GuacamoleException
+     *     If an error occurs parsing the configuration properties used by this
+     *     extension.
      */
-    public static void setAuthenticationFailureTracker(AuthenticationFailureTracker tracker) {
-        BanningAuthenticationListener.tracker = tracker;
+    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 void handleEvent(Object event) throws GuacamoleException {
 
         }
 
+        // ... and explicit success.
         else if (event instanceof AuthenticationSuccessEvent) {
             AuthenticationSuccessEvent success = (AuthenticationSuccessEvent) event;
             tracker.notifyAuthenticationSuccess(success.getCredentials());

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