From 2ed031712918313b50483d22cf6735aed227dc06 Mon Sep 17 00:00:00 2001 From: Dan Brown Date: Tue, 28 Jul 2020 12:59:43 +0100 Subject: [PATCH] Updated functionality for logging failed access - Added testing to cover. - Linked logging into Laravel's monolog logging system and made log channel configurable. - Updated env var names to be specific to login access. - Added extra locations as to where failed logins would be captured. Related to #1881 and #728 --- .env.example.complete | 12 ++++---- app/Actions/ActivityService.php | 30 +++++++++---------- app/Config/logging.php | 26 +++++++++++++--- app/Http/Controllers/Auth/LoginController.php | 10 +++---- phpunit.xml | 2 ++ tests/Auth/AuthTest.php | 12 ++++++++ tests/Auth/LdapTest.php | 13 ++++++++ tests/Unit/ConfigTest.php | 23 ++++++++++++++ 8 files changed, 98 insertions(+), 30 deletions(-) diff --git a/.env.example.complete b/.env.example.complete index 26dfdc54b..d7dd02b9a 100644 --- a/.env.example.complete +++ b/.env.example.complete @@ -272,8 +272,10 @@ API_MAX_ITEM_COUNT=500 # The number of API requests that can be made per minute by a single user. API_REQUESTS_PER_MIN=180 -# Failed access -# message to log into webserver logs in case of failed access, for further processing by tools like Fail2Ban -# Apache users should use : user "%u" authentication failure for "BookStack" -# Nginx users should use : user "%u" was not found in "BookStack" -FAILED_ACCESS_MESSAGE='' +# Enable the logging of failed email+password logins with the given message +# The defaul log channel below uses the php 'error_log' function which commonly +# results in messages being output to the webserver error logs. +# The message can contain a %u parameter which will be replaced with the login +# user identifier (Username or email). +LOG_FAILED_LOGIN_MESSAGE=false +LOG_FAILED_LOGIN_CHANNEL=errorlog_plain_webserver diff --git a/app/Actions/ActivityService.php b/app/Actions/ActivityService.php index 0e3ac7861..e6b004f01 100644 --- a/app/Actions/ActivityService.php +++ b/app/Actions/ActivityService.php @@ -4,6 +4,7 @@ use BookStack\Auth\Permissions\PermissionService; use BookStack\Auth\User; use BookStack\Entities\Entity; use Illuminate\Support\Collection; +use Illuminate\Support\Facades\Log; class ActivityService { @@ -49,7 +50,7 @@ class ActivityService protected function newActivityForUser(string $key, ?int $bookId = null): Activity { return $this->activity->newInstance()->forceFill([ - 'key' => strtolower($key), + 'key' => strtolower($key), 'user_id' => $this->user->id, 'book_id' => $bookId ?? 0, ]); @@ -64,8 +65,8 @@ class ActivityService { $activities = $entity->activity()->get(); $entity->activity()->update([ - 'extra' => $entity->name, - 'entity_id' => 0, + 'extra' => $entity->name, + 'entity_id' => 0, 'entity_type' => '', ]); return $activities; @@ -99,7 +100,7 @@ class ActivityService $query = $this->activity->newQuery()->where('entity_type', '=', $entity->getMorphClass()) ->where('entity_id', '=', $entity->id); } - + $activity = $this->permissionService ->filterRestrictedEntityRelations($query, 'activities', 'entity_id', 'entity_type') ->orderBy('created_at', 'desc') @@ -161,19 +162,18 @@ class ActivityService } /** - * Log failed accesses, for further processing by tools like Fail2Ban - * - * @param username - * @return void - */ - public function logFailedAccess($username) + * Log out a failed login attempt, Providing the given username + * as part of the message if the '%u' string is used. + */ + public function logFailedLogin(string $username) { - $log_msg = config('logging.failed_access_message'); - - if (!is_string($username) || !is_string($log_msg) || strlen($log_msg)<1) + $message = config('logging.failed_login.message'); + if (!$message) { return; + } - $log_msg = str_replace("%u", $username, $log_msg); - error_log($log_msg, 4); + $message = str_replace("%u", $username, $message); + $channel = config('logging.failed_login.channel'); + Log::channel($channel)->warning($message); } } diff --git a/app/Config/logging.php b/app/Config/logging.php index ba77ba81e..afd56e482 100644 --- a/app/Config/logging.php +++ b/app/Config/logging.php @@ -1,5 +1,7 @@ 'debug', ], + // Custom errorlog implementation that logs out a plain, + // non-formatted message intended for the webserver log. + 'errorlog_plain_webserver' => [ + 'driver' => 'monolog', + 'level' => 'debug', + 'handler' => ErrorLogHandler::class, + 'handler_with' => [4], + 'formatter' => LineFormatter::class, + 'formatter_with' => [ + 'format' => "%message%", + ], + ], + 'null' => [ 'driver' => 'monolog', 'handler' => NullHandler::class, @@ -86,9 +101,12 @@ return [ ], ], - // Failed Access Message - // Defines the message to log into webserver logs in case of failed access, - // for further processing by tools like Fail2Ban. - 'failed_access_message' => env('FAILED_ACCESS_MESSAGE', ''), + + // Failed Login Message + // Allows a configurable message to be logged when a login request fails. + 'failed_login' => [ + 'message' => env('LOG_FAILED_LOGIN_MESSAGE', null), + 'channel' => env('LOG_FAILED_LOGIN_CHANNEL', 'errorlog_plain_webserver'), + ], ]; diff --git a/app/Http/Controllers/Auth/LoginController.php b/app/Http/Controllers/Auth/LoginController.php index f031c12cf..cd7a4db32 100644 --- a/app/Http/Controllers/Auth/LoginController.php +++ b/app/Http/Controllers/Auth/LoginController.php @@ -99,6 +99,7 @@ class LoginController extends Controller public function login(Request $request) { $this->validateLogin($request); + $username = $request->get($this->username()); // If the class is using the ThrottlesLogins trait, we can automatically throttle // the login attempts for this application. We'll key this by the username and @@ -107,9 +108,7 @@ class LoginController extends Controller $this->hasTooManyLoginAttempts($request)) { $this->fireLockoutEvent($request); - // Also log some error message - Activity::logFailedAccess($request->get($this->username())); - + Activity::logFailedLogin($username); return $this->sendLockoutResponse($request); } @@ -118,6 +117,7 @@ class LoginController extends Controller return $this->sendLoginResponse($request); } } catch (LoginAttemptException $exception) { + Activity::logFailedLogin($username); return $this->sendLoginAttemptExceptionResponse($exception, $request); } @@ -126,9 +126,7 @@ class LoginController extends Controller // user surpasses their maximum number of attempts they will get locked out. $this->incrementLoginAttempts($request); - // Also log some error message - Activity::logFailedAccess($request->get($this->username())); - + Activity::logFailedLogin($username); return $this->sendFailedLoginResponse($request); } diff --git a/phpunit.xml b/phpunit.xml index 85538c446..70f1c1f9c 100644 --- a/phpunit.xml +++ b/phpunit.xml @@ -51,5 +51,7 @@ + + diff --git a/tests/Auth/AuthTest.php b/tests/Auth/AuthTest.php index f1f476966..8900eeeba 100644 --- a/tests/Auth/AuthTest.php +++ b/tests/Auth/AuthTest.php @@ -401,6 +401,18 @@ class AuthTest extends BrowserKitTest $this->assertFalse(auth('saml2')->check()); } + public function test_failed_logins_are_logged_when_message_configured() + { + $log = $this->withTestLogger(); + config()->set(['logging.failed_login.message' => 'Failed login for %u']); + + $this->post('/login', ['email' => 'admin@example.com', 'password' => 'cattreedog']); + $this->assertTrue($log->hasWarningThatContains('Failed login for admin@example.com')); + + $this->post('/login', ['email' => 'admin@admin.com', 'password' => 'password']); + $this->assertFalse($log->hasWarningThatContains('Failed login for admin@admin.com')); + } + /** * Perform a login */ diff --git a/tests/Auth/LdapTest.php b/tests/Auth/LdapTest.php index ed8748f08..0d92241bb 100644 --- a/tests/Auth/LdapTest.php +++ b/tests/Auth/LdapTest.php @@ -593,4 +593,17 @@ class LdapTest extends BrowserKitTest $this->see('A user with the email tester@example.com already exists but with different credentials'); } + + public function test_failed_logins_are_logged_when_message_configured() + { + $log = $this->withTestLogger(); + config()->set(['logging.failed_login.message' => 'Failed login for %u']); + + $this->commonLdapMocks(1, 1, 1, 1, 1); + $this->mockLdap->shouldReceive('searchAndGetEntries')->times(1) + ->andReturn(['count' => 0]); + + $this->post('/login', ['username' => 'timmyjenkins', 'password' => 'cattreedog']); + $this->assertTrue($log->hasWarningThatContains('Failed login for timmyjenkins')); + } } diff --git a/tests/Unit/ConfigTest.php b/tests/Unit/ConfigTest.php index 69b737d7d..1374b3aa9 100644 --- a/tests/Unit/ConfigTest.php +++ b/tests/Unit/ConfigTest.php @@ -1,5 +1,6 @@ checkEnvConfigResult('APP_URL', $oldDefault, 'app.url', ''); } + public function test_errorlog_plain_webserver_channel() + { + // We can't full test this due to it being targeted for the SAPI logging handler + // so we just overwrite that component so we can capture the error log output. + config()->set([ + 'logging.channels.errorlog_plain_webserver.handler_with' => [0], + ]); + + $temp = tempnam(sys_get_temp_dir(), 'bs-test'); + $original = ini_set( 'error_log', $temp); + + Log::channel('errorlog_plain_webserver')->info('Aww, look, a cute puppy'); + + ini_set( 'error_log', $original); + + $output = file_get_contents($temp); + $this->assertStringContainsString('Aww, look, a cute puppy', $output); + $this->assertStringNotContainsString('INFO', $output); + $this->assertStringNotContainsString('info', $output); + $this->assertStringNotContainsString('testing', $output); + } + /** * Set an environment variable of the given name and value * then check the given config key to see if it matches the given result.