From 44acf706812bc77812e6648c2cc28af36e172a14 Mon Sep 17 00:00:00 2001 From: VirtualTam Date: Wed, 25 Oct 2017 23:03:31 +0200 Subject: [PATCH] Refactor login / ban authentication steps Relates to https://github.com/shaarli/Shaarli/issues/324 Added: - Add the `LoginManager` class to manage logins and bans Changed: - Refactor IP ban management - Simplify logic - Avoid using globals, inject dependencies Fixed: - Use `ban_duration` instead of `ban_after` when setting a new ban Signed-off-by: VirtualTam --- application/LoginManager.php | 134 ++++++++++++++++++++ index.php | 116 +++-------------- tests/LoginManagerTest.php | 199 ++++++++++++++++++++++++++++++ tests/utils/FakeConfigManager.php | 35 +++++- tpl/default/loginform.html | 2 +- tpl/vintage/loginform.html | 2 +- 6 files changed, 385 insertions(+), 103 deletions(-) create mode 100644 application/LoginManager.php create mode 100644 tests/LoginManagerTest.php diff --git a/application/LoginManager.php b/application/LoginManager.php new file mode 100644 index 00000000..397bc6e3 --- /dev/null +++ b/application/LoginManager.php @@ -0,0 +1,134 @@ +globals = &$globals; + $this->configManager = $configManager; + $this->banFile = $this->configManager->get('resource.ban_file', 'data/ipbans.php'); + $this->readBanFile(); + } + + /** + * 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, + "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(); + } + + /** + * Handle a successful login + * + * @param array $server The $_SERVER array + */ + 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(); + } + + /** + * Check if the user can login from this IP + * + * @param array $server The $_SERVER array + * + * @return bool true if the user is allowed to login + */ + 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; + } +} diff --git a/index.php b/index.php index 067b8fcb..91c3f07e 100644 --- a/index.php +++ b/index.php @@ -78,6 +78,7 @@ require_once 'application/Updater.php'; use \Shaarli\Languages; use \Shaarli\ThemeUtils; use \Shaarli\Config\ConfigManager; +use \Shaarli\LoginManager; use \Shaarli\SessionManager; // Ensure the PHP version is supported @@ -122,6 +123,7 @@ if (isset($_COOKIE['shaarli']) && !SessionManager::checkId($_COOKIE['shaarli'])) } $conf = new ConfigManager(); +$loginManager = new LoginManager($GLOBALS, $conf); $sessionManager = new SessionManager($_SESSION, $conf); // LC_MESSAGES isn't defined without php-intl, in this case use LC_COLLATE locale instead. @@ -293,108 +295,22 @@ function logout() { setcookie('shaarli_staySignedIn', FALSE, 0, WEB_PATH); } - -// ------------------------------------------------------------------------------------------ -// Brute force protection system -// Several consecutive failed logins will ban the IP address for 30 minutes. -if (!is_file($conf->get('resource.ban_file', 'data/ipbans.php'))) { - // FIXME! globals - file_put_contents( - $conf->get('resource.ban_file', 'data/ipbans.php'), - "array(),'BANS'=>array()),true).";\n?>" - ); -} -include $conf->get('resource.ban_file', 'data/ipbans.php'); -/** - * Signal a failed login. Will ban the IP if too many failures: - * - * @param ConfigManager $conf Configuration Manager instance. - */ -function ban_loginFailed($conf) -{ - $ip = $_SERVER['REMOTE_ADDR']; - $trusted = $conf->get('security.trusted_proxies', array()); - if (in_array($ip, $trusted)) { - $ip = getIpAddressFromProxy($_SERVER, $trusted); - if (!$ip) { - return; - } - } - $gb = $GLOBALS['IPBANS']; - if (! isset($gb['FAILURES'][$ip])) { - $gb['FAILURES'][$ip]=0; - } - $gb['FAILURES'][$ip]++; - if ($gb['FAILURES'][$ip] > ($conf->get('security.ban_after') - 1)) - { - $gb['BANS'][$ip] = time() + $conf->get('security.ban_after', 1800); - logm($conf->get('resource.log'), $_SERVER['REMOTE_ADDR'], 'IP address banned from login'); - } - $GLOBALS['IPBANS'] = $gb; - file_put_contents( - $conf->get('resource.ban_file', 'data/ipbans.php'), - "" - ); -} - -/** - * Signals a successful login. Resets failed login counter. - * - * @param ConfigManager $conf Configuration Manager instance. - */ -function ban_loginOk($conf) -{ - $ip = $_SERVER['REMOTE_ADDR']; - $gb = $GLOBALS['IPBANS']; - unset($gb['FAILURES'][$ip]); unset($gb['BANS'][$ip]); - $GLOBALS['IPBANS'] = $gb; - file_put_contents( - $conf->get('resource.ban_file', 'data/ipbans.php'), - "" - ); -} - -/** - * Checks if the user CAN login. If 'true', the user can try to login. - * - * @param ConfigManager $conf Configuration Manager instance. - * - * @return bool: true if the user is allowed to login. - */ -function ban_canLogin($conf) -{ - $ip=$_SERVER["REMOTE_ADDR"]; $gb=$GLOBALS['IPBANS']; - if (isset($gb['BANS'][$ip])) - { - // User is banned. Check if the ban has expired: - if ($gb['BANS'][$ip]<=time()) - { // Ban expired, user can try to login again. - logm($conf->get('resource.log'), $_SERVER['REMOTE_ADDR'], 'Ban lifted.'); - unset($gb['FAILURES'][$ip]); unset($gb['BANS'][$ip]); - file_put_contents( - $conf->get('resource.ban_file', 'data/ipbans.php'), - "" - ); - return true; // Ban has expired, user can login. - } - return false; // User is banned. - } - return true; // User is not banned. -} - // ------------------------------------------------------------------------------------------ // Process login form: Check if login/password is correct. if (isset($_POST['login'])) { - if (!ban_canLogin($conf)) die(t('I said: NO. You are banned for the moment. Go away.')); + if (! $loginManager->canLogin($_SERVER)) { + die(t('I said: NO. You are banned for the moment. Go away.')); + } if (isset($_POST['password']) && $sessionManager->checkToken($_POST['token']) && (check_auth($_POST['login'], $_POST['password'], $conf)) - ) { // Login/password is OK. - ban_loginOk($conf); + ) { + // Login/password is OK. + $loginManager->handleSuccessfulLogin($_SERVER); + // If user wants to keep the session cookie even after the browser closes: - if (!empty($_POST['longlastingsession'])) - { + if (!empty($_POST['longlastingsession'])) { $_SESSION['longlastingsession'] = 31536000; // (31536000 seconds = 1 year) $expiration = time() + $_SESSION['longlastingsession']; // calculate relative cookie expiration (1 year from now) setcookie('shaarli_staySignedIn', STAY_SIGNED_IN_TOKEN, $expiration, WEB_PATH); @@ -437,10 +353,8 @@ if (isset($_POST['login'])) } } header('Location: ?'); exit; - } - else - { - ban_loginFailed($conf); + } else { + $loginManager->handleFailedLogin($_SERVER); $redir = '&username='. urlencode($_POST['login']); if (isset($_GET['post'])) { $redir .= '&post=' . urlencode($_GET['post']); @@ -684,8 +598,9 @@ function showLinkList($PAGE, $LINKSDB, $conf, $pluginManager) { * @param LinkDB $LINKSDB * @param History $history instance * @param SessionManager $sessionManager SessionManager instance + * @param LoginManager $loginManager LoginManager instance */ -function renderPage($conf, $pluginManager, $LINKSDB, $history, $sessionManager) +function renderPage($conf, $pluginManager, $LINKSDB, $history, $sessionManager, $loginManager) { $updater = new Updater( read_updates_file($conf->get('resource.updates')), @@ -761,6 +676,7 @@ function renderPage($conf, $pluginManager, $LINKSDB, $history, $sessionManager) $PAGE->assign('returnurl',(isset($_SERVER['HTTP_REFERER']) ? escape($_SERVER['HTTP_REFERER']):'')); // add default state of the 'remember me' checkbox $PAGE->assign('remember_user_default', $conf->get('privacy.remember_user_default')); + $PAGE->assign('user_can_login', $loginManager->canLogin($_SERVER)); $PAGE->renderPage('loginform'); exit; } @@ -2330,7 +2246,7 @@ $response = $app->run(true); if ($response->getStatusCode() == 404 && strpos($_SERVER['REQUEST_URI'], '/api/v1') === false) { // We use UTF-8 for proper international characters handling. header('Content-Type: text/html; charset=utf-8'); - renderPage($conf, $pluginManager, $linkDb, $history, $sessionManager); + renderPage($conf, $pluginManager, $linkDb, $history, $sessionManager, $loginManager); } else { $app->respond($response); } diff --git a/tests/LoginManagerTest.php b/tests/LoginManagerTest.php new file mode 100644 index 00000000..4159038e --- /dev/null +++ b/tests/LoginManagerTest.php @@ -0,0 +1,199 @@ +banFile)) { + unlink($this->banFile); + } + + $this->configManager = new \FakeConfigManager([ + 'resource.ban_file' => $this->banFile, + 'resource.log' => $this->logFile, + 'security.ban_after' => 4, + 'security.ban_duration' => 3600, + 'security.trusted_proxies' => [$this->trustedProxy], + ]); + + $this->globals = &$GLOBALS; + unset($this->globals['IPBANS']); + + $this->loginManager = new LoginManager($this->globals, $this->configManager); + $this->server['REMOTE_ADDR'] = $this->ipAddr; + } + + /** + * Wipe test resources + */ + public function tearDown() + { + unset($this->globals['IPBANS']); + } + + /** + * Instantiate a LoginManager and load ban records + */ + public function testReadBanFile() + { + file_put_contents( + $this->banFile, + " array('127.0.0.1' => 99));\n?>" + ); + new LoginManager($this->globals, $this->configManager); + $this->assertEquals(99, $this->globals['IPBANS']['FAILURES']['127.0.0.1']); + } + + /** + * Record a failed login attempt + */ + public function testHandleFailedLogin() + { + $this->loginManager->handleFailedLogin($this->server); + $this->assertEquals(1, $this->globals['IPBANS']['FAILURES'][$this->ipAddr]); + + $this->loginManager->handleFailedLogin($this->server); + $this->assertEquals(2, $this->globals['IPBANS']['FAILURES'][$this->ipAddr]); + } + + /** + * Record a failed login attempt - IP behind a trusted proxy + */ + public function testHandleFailedLoginBehindTrustedProxy() + { + $server = [ + 'REMOTE_ADDR' => $this->trustedProxy, + 'HTTP_X_FORWARDED_FOR' => $this->ipAddr, + ]; + $this->loginManager->handleFailedLogin($server); + $this->assertEquals(1, $this->globals['IPBANS']['FAILURES'][$this->ipAddr]); + + $this->loginManager->handleFailedLogin($server); + $this->assertEquals(2, $this->globals['IPBANS']['FAILURES'][$this->ipAddr]); + } + + /** + * Record a failed login attempt - IP behind a trusted proxy but not forwarded + */ + public function testHandleFailedLoginBehindTrustedProxyNoIp() + { + $server = [ + 'REMOTE_ADDR' => $this->trustedProxy, + ]; + $this->loginManager->handleFailedLogin($server); + $this->assertFalse(isset($this->globals['IPBANS']['FAILURES'][$this->ipAddr])); + + $this->loginManager->handleFailedLogin($server); + $this->assertFalse(isset($this->globals['IPBANS']['FAILURES'][$this->ipAddr])); + } + + /** + * Record a failed login attempt and ban the IP after too many failures + */ + public function testHandleFailedLoginBanIp() + { + $this->loginManager->handleFailedLogin($this->server); + $this->assertEquals(1, $this->globals['IPBANS']['FAILURES'][$this->ipAddr]); + $this->assertTrue($this->loginManager->canLogin($this->server)); + + $this->loginManager->handleFailedLogin($this->server); + $this->assertEquals(2, $this->globals['IPBANS']['FAILURES'][$this->ipAddr]); + $this->assertTrue($this->loginManager->canLogin($this->server)); + + $this->loginManager->handleFailedLogin($this->server); + $this->assertEquals(3, $this->globals['IPBANS']['FAILURES'][$this->ipAddr]); + $this->assertTrue($this->loginManager->canLogin($this->server)); + + $this->loginManager->handleFailedLogin($this->server); + $this->assertEquals(4, $this->globals['IPBANS']['FAILURES'][$this->ipAddr]); + $this->assertFalse($this->loginManager->canLogin($this->server)); + + // handleFailedLogin is not supposed to be called at this point: + // - no login form should be displayed once an IP has been banned + // - yet this could happen when using custom templates / scripts + $this->loginManager->handleFailedLogin($this->server); + $this->assertEquals(5, $this->globals['IPBANS']['FAILURES'][$this->ipAddr]); + $this->assertFalse($this->loginManager->canLogin($this->server)); + } + + /** + * Nothing to do + */ + public function testHandleSuccessfulLogin() + { + $this->assertTrue($this->loginManager->canLogin($this->server)); + + $this->loginManager->handleSuccessfulLogin($this->server); + $this->assertTrue($this->loginManager->canLogin($this->server)); + } + + /** + * Erase failure records after successfully logging in from this IP + */ + public function testHandleSuccessfulLoginAfterFailure() + { + $this->loginManager->handleFailedLogin($this->server); + $this->loginManager->handleFailedLogin($this->server); + $this->assertEquals(2, $this->globals['IPBANS']['FAILURES'][$this->ipAddr]); + $this->assertTrue($this->loginManager->canLogin($this->server)); + + $this->loginManager->handleSuccessfulLogin($this->server); + $this->assertTrue($this->loginManager->canLogin($this->server)); + $this->assertFalse(isset($this->globals['IPBANS']['FAILURES'][$this->ipAddr])); + $this->assertFalse(isset($this->globals['IPBANS']['BANS'][$this->ipAddr])); + } + + /** + * The IP is not banned + */ + public function testCanLoginIpNotBanned() + { + $this->assertTrue($this->loginManager->canLogin($this->server)); + } + + /** + * The IP is banned + */ + public function testCanLoginIpBanned() + { + // ban the IP for an hour + $this->globals['IPBANS']['FAILURES'][$this->ipAddr] = 10; + $this->globals['IPBANS']['BANS'][$this->ipAddr] = time() + 3600; + + $this->assertFalse($this->loginManager->canLogin($this->server)); + } + + /** + * The IP is banned, and the ban duration is over + */ + public function testCanLoginIpBanExpired() + { + // ban the IP for an hour + $this->globals['IPBANS']['FAILURES'][$this->ipAddr] = 10; + $this->globals['IPBANS']['BANS'][$this->ipAddr] = time() + 3600; + $this->assertFalse($this->loginManager->canLogin($this->server)); + + // lift the ban + $this->globals['IPBANS']['BANS'][$this->ipAddr] = time() - 3600; + $this->assertTrue($this->loginManager->canLogin($this->server)); + } +} diff --git a/tests/utils/FakeConfigManager.php b/tests/utils/FakeConfigManager.php index f29760cb..85434de7 100644 --- a/tests/utils/FakeConfigManager.php +++ b/tests/utils/FakeConfigManager.php @@ -5,8 +5,41 @@ */ class FakeConfigManager { - public static function get($key) + protected $values = []; + + /** + * Initialize with test values + * + * @param array $values Initial values + */ + public function __construct($values = []) + { + $this->values = $values; + } + + /** + * Set a given value + * + * @param string $key Key of the value to set + * @param mixed $value Value to set + */ + public function set($key, $value) + { + $this->values[$key] = $value; + } + + /** + * Get a given configuration value + * + * @param string $key Index of the value to retrieve + * + * @return mixed The value if set, else the name of the key + */ + public function get($key) { + if (isset($this->values[$key])) { + return $this->values[$key]; + } return $key; } } diff --git a/tpl/default/loginform.html b/tpl/default/loginform.html index 5777a218..d481f452 100644 --- a/tpl/default/loginform.html +++ b/tpl/default/loginform.html @@ -5,7 +5,7 @@ {include="page.header"} -{if="!ban_canLogin($conf)"} +{if="!$user_can_login"}
diff --git a/tpl/vintage/loginform.html b/tpl/vintage/loginform.html index 1becd44f..2c9b710e 100644 --- a/tpl/vintage/loginform.html +++ b/tpl/vintage/loginform.html @@ -2,7 +2,7 @@ {include="includes"}