Merge pull request #1273 from ArthurHoaro/feature/ban-manager

Rewrite IP ban management

2 files changed, 227 insertions, 86 deletions

diff --git a/application/security/BanManager.php b/application/security/BanManager.php
new file mode 100644
index 00000000..68190c54
--- /dev/null
+++ b/application/security/BanManager.php
@@ -0,0 +1,213 @@
+<?php
+
+
+namespace Shaarli\Security;
+
+use Shaarli\FileUtils;
+
+/**
+ * Class BanManager
+ *
+ * Failed login attempts will store the associated IP address.
+ * After N failed attempts, the IP will be prevented from log in for duration D.
+ * Both N and D can be set in the configuration file.
+ *
+ * @package Shaarli\Security
+ */
+class BanManager
+{
+    /** @var array List of allowed proxies IP */
+    protected $trustedProxies;
+
+    /** @var int Number of allowed failed attempt before the ban */
+    protected $nbAttempts;
+
+    /** @var  int Ban duration in seconds */
+    protected $banDuration;
+
+    /** @var string Path to the file containing IP bans and failures */
+    protected $banFile;
+
+    /** @var string Path to the log file, used to log bans */
+    protected $logFile;
+
+    /** @var array List of IP with their associated number of failed attempts */
+    protected $failures = [];
+
+    /** @var array List of banned IP with their associated unban timestamp */
+    protected $bans = [];
+
+    /**
+     * BanManager constructor.
+     *
+     * @param array  $trustedProxies List of allowed proxies IP
+     * @param int    $nbAttempts     Number of allowed failed attempt before the ban
+     * @param int    $banDuration    Ban duration in seconds
+     * @param string $banFile        Path to the file containing IP bans and failures
+     * @param string $logFile        Path to the log file, used to log bans
+     */
+    public function __construct($trustedProxies, $nbAttempts, $banDuration, $banFile, $logFile) {
+        $this->trustedProxies = $trustedProxies;
+        $this->nbAttempts = $nbAttempts;
+        $this->banDuration = $banDuration;
+        $this->banFile = $banFile;
+        $this->logFile = $logFile;
+        $this->readBanFile();
+    }
+
+    /**
+     * Handle a failed login and ban the IP after too many failed attempts
+     *
+     * @param array $server The $_SERVER array
+     */
+    public function handleFailedAttempt($server)
+    {
+        $ip = $this->getIp($server);
+        // the IP is behind a trusted forward proxy, but is not forwarded
+        // in the HTTP headers, so we do nothing
+        if (empty($ip)) {
+            return;
+        }
+
+        // increment the fail count for this IP
+        if (isset($this->failures[$ip])) {
+            $this->failures[$ip]++;
+        } else {
+            $this->failures[$ip] = 1;
+        }
+
+        if ($this->failures[$ip] >= $this->nbAttempts) {
+            $this->bans[$ip] = time() + $this->banDuration;
+            logm(
+                $this->logFile,
+                $server['REMOTE_ADDR'],
+                'IP address banned from login: '. $ip
+            );
+        }
+        $this->writeBanFile();
+    }
+
+    /**
+     * Remove failed attempts for the provided client.
+     *
+     * @param array $server $_SERVER
+     */
+    public function clearFailures($server)
+    {
+        $ip = $this->getIp($server);
+        // the IP is behind a trusted forward proxy, but is not forwarded
+        // in the HTTP headers, so we do nothing
+        if (empty($ip)) {
+            return;
+        }
+
+        if (isset($this->failures[$ip])) {
+            unset($this->failures[$ip]);
+        }
+        $this->writeBanFile();
+    }
+
+    /**
+     * Check whether the client IP is banned or not.
+     *
+     * @param array $server $_SERVER
+     *
+     * @return bool True if the IP is banned, false otherwise
+     */
+    public function isBanned($server)
+    {
+        $ip = $this->getIp($server);
+        // the IP is behind a trusted forward proxy, but is not forwarded
+        // in the HTTP headers, so we allow the authentication attempt.
+        if (empty($ip)) {
+            return false;
+        }
+
+        // the user is not banned
+        if (! isset($this->bans[$ip])) {
+            return false;
+        }
+
+        // the user is still banned
+        if ($this->bans[$ip] > time()) {
+            return true;
+        }
+
+        // the ban has expired, the user can attempt to log in again
+        if (isset($this->failures[$ip])) {
+            unset($this->failures[$ip]);
+        }
+        unset($this->bans[$ip]);
+        logm($this->logFile, $server['REMOTE_ADDR'], 'Ban lifted for: '. $ip);
+
+        $this->writeBanFile();
+        return false;
+    }
+
+    /**
+     * Retrieve the IP from $_SERVER.
+     * If the actual IP is behind an allowed reverse proxy,
+     * we try to extract the forwarded IP from HTTP headers.
+     *
+     * @param array $server $_SERVER
+     *
+     * @return string|bool The IP or false if none could be extracted
+     */
+    protected function getIp($server)
+    {
+        $ip = $server['REMOTE_ADDR'];
+        if (! in_array($ip, $this->trustedProxies)) {
+            return $ip;
+        }
+        return getIpAddressFromProxy($server, $this->trustedProxies);
+    }
+
+    /**
+     * Read a file containing banned IPs
+     */
+    protected function readBanFile()
+    {
+        $data = FileUtils::readFlatDB($this->banFile);
+        if (isset($data['failures']) && is_array($data['failures'])) {
+            $this->failures = $data['failures'];
+        }
+
+        if (isset($data['bans']) && is_array($data['bans'])) {
+            $this->bans = $data['bans'];
+        }
+    }
+
+    /**
+     * Write the banned IPs to a file
+     */
+    protected function writeBanFile()
+    {
+        return FileUtils::writeFlatDB(
+            $this->banFile,
+            [
+                'failures' => $this->failures,
+                'bans' => $this->bans,
+            ]
+        );
+    }
+
+    /**
+     * Get the Failures (for UT purpose).
+     *
+     * @return array
+     */
+    public function getFailures()
+    {
+        return $this->failures;
+    }
+
+    /**
+     * Get the Bans (for UT purpose).
+     *
+     * @return array
+     */
+    public function getBans()
+    {
+        return $this->bans;
+    }
+}
diff --git a/application/security/LoginManager.php b/application/security/LoginManager.php
index 1ff3d0be..0b0ce0b1 100644
--- a/application/security/LoginManager.php
+++ b/application/security/LoginManager.php
@@ -20,8 +20,8 @@ class LoginManager
     /** @var SessionManager Session Manager instance **/
     protected $sessionManager = null;
 
-    /** @var string Path to the file containing IP bans */
-    protected $banFile = '';
+    /** @var BanManager Ban Manager instance **/
+    protected $banManager;
 
     /** @var bool Whether the user is logged in **/
     protected $isLoggedIn = false;
@@ -35,17 +35,21 @@ class LoginManager
     /**
      * Constructor
      *
-     * @param array          $globals        The $GLOBALS array (reference)
      * @param ConfigManager  $configManager  Configuration Manager instance
      * @param SessionManager $sessionManager SessionManager instance
      */
-    public function __construct(& $globals, $configManager, $sessionManager)
+    public function __construct($configManager, $sessionManager)
     {
-        $this->globals = &$globals;
         $this->configManager = $configManager;
         $this->sessionManager = $sessionManager;
-        $this->banFile = $this->configManager->get('resource.ban_file', 'data/ipbans.php');
-        $this->readBanFile();
+        $this->banManager = new BanManager(
+            $this->configManager->get('security.trusted_proxies', []),
+            $this->configManager->get('security.ban_after'),
+            $this->configManager->get('security.ban_duration'),
+            $this->configManager->get('resource.ban_file', 'data/ipbans.php'),
+            $this->configManager->get('resource.log')
+        );
+
         if ($this->configManager->get('security.open_shaarli') === true) {
             $this->openShaarli = true;
         }
@@ -158,65 +162,13 @@ class LoginManager
     }
 
     /**
-     * Read a file containing banned IPs
-     */
-    protected function readBanFile()
-    {
-        if (! file_exists($this->banFile)) {
-            return;
-        }
-        include $this->banFile;
-    }
-
-    /**
-     * Write the banned IPs to a file
-     */
-    protected function writeBanFile()
-    {
-        if (! array_key_exists('IPBANS', $this->globals)) {
-            return;
-        }
-        file_put_contents(
-            $this->banFile,
-            "<?php\n\$GLOBALS['IPBANS']=" . var_export($this->globals['IPBANS'], true) . ";\n?>"
-        );
-    }
-
-    /**
      * Handle a failed login and ban the IP after too many failed attempts
      *
      * @param array $server The $_SERVER array
      */
     public function handleFailedLogin($server)
     {
-        $ip = $server['REMOTE_ADDR'];
-        $trusted = $this->configManager->get('security.trusted_proxies', []);
-
-        if (in_array($ip, $trusted)) {
-            $ip = getIpAddressFromProxy($server, $trusted);
-            if (! $ip) {
-                // the IP is behind a trusted forward proxy, but is not forwarded
-                // in the HTTP headers, so we do nothing
-                return;
-            }
-        }
-
-        // increment the fail count for this IP
-        if (isset($this->globals['IPBANS']['FAILURES'][$ip])) {
-            $this->globals['IPBANS']['FAILURES'][$ip]++;
-        } else {
-            $this->globals['IPBANS']['FAILURES'][$ip] = 1;
-        }
-
-        if ($this->globals['IPBANS']['FAILURES'][$ip] >= $this->configManager->get('security.ban_after')) {
-            $this->globals['IPBANS']['BANS'][$ip] = time() + $this->configManager->get('security.ban_duration', 1800);
-            logm(
-                $this->configManager->get('resource.log'),
-                $server['REMOTE_ADDR'],
-                'IP address banned from login'
-            );
-        }
-        $this->writeBanFile();
+        $this->banManager->handleFailedAttempt($server);
     }
 
     /**
@@ -226,13 +178,7 @@ class LoginManager
      */
     public function handleSuccessfulLogin($server)
     {
-        $ip = $server['REMOTE_ADDR'];
-        // FIXME unban when behind a trusted proxy?
-
-        unset($this->globals['IPBANS']['FAILURES'][$ip]);
-        unset($this->globals['IPBANS']['BANS'][$ip]);
-
-        $this->writeBanFile();
+        $this->banManager->clearFailures($server);
     }
 
     /**
@@ -244,24 +190,6 @@ class LoginManager
      */
     public function canLogin($server)
     {
-        $ip = $server['REMOTE_ADDR'];
-
-        if (! isset($this->globals['IPBANS']['BANS'][$ip])) {
-            // the user is not banned
-            return true;
-        }
-
-        if ($this->globals['IPBANS']['BANS'][$ip] > time()) {
-            // the user is still banned
-            return false;
-        }
-
-        // the ban has expired, the user can attempt to log in again
-        logm($this->configManager->get('resource.log'), $server['REMOTE_ADDR'], 'Ban lifted.');
-        unset($this->globals['IPBANS']['FAILURES'][$ip]);
-        unset($this->globals['IPBANS']['BANS'][$ip]);
-
-        $this->writeBanFile();
-        return true;
+        return ! $this->banManager->isBanned($server);
     }
 }