From af032f89932290b9f1217e80733447da6c039f55 Mon Sep 17 00:00:00 2001 From: Dan Brown Date: Sun, 7 Feb 2021 20:00:04 +0000 Subject: [PATCH] Tweaked LDAP TLS Implementation - Moved the ldap function out to our separate service for easier testing. - Added testing for the option. - Moved tls_insecure part back up above connection start as found more reliable there. Done a lot of real-connection testing during this review. Used wireshare to ensure TLS connection does take place. Found LDAP_TLS_INSECURE=false can action unreliably, restarting php-fpm helped. Tested both trusted and untrusted certificates. --- app/Auth/Access/Ldap.php | 8 ++++++++ app/Auth/Access/LdapService.php | 33 ++++++++++++++++----------------- phpunit.xml | 1 + tests/Auth/LdapTest.php | 32 ++++++++++++++++++++++++++------ 4 files changed, 51 insertions(+), 23 deletions(-) diff --git a/app/Auth/Access/Ldap.php b/app/Auth/Access/Ldap.php index 6b7bd9b9b..352231df5 100644 --- a/app/Auth/Access/Ldap.php +++ b/app/Auth/Access/Ldap.php @@ -31,6 +31,14 @@ class Ldap return ldap_set_option($ldapConnection, $option, $value); } + /** + * Start TLS on the given LDAP connection. + */ + public function startTls($ldapConnection): bool + { + return ldap_start_tls($ldapConnection); + } + /** * Set the version number for the given ldap connection. * @param $ldapConnection diff --git a/app/Auth/Access/LdapService.php b/app/Auth/Access/LdapService.php index c359cc943..a438c0984 100644 --- a/app/Auth/Access/LdapService.php +++ b/app/Auth/Access/LdapService.php @@ -85,9 +85,9 @@ class LdapService extends ExternalAuthService $userCn = $this->getUserResponseProperty($user, 'cn', null); $formatted = [ - 'uid' => $this->getUserResponseProperty($user, $idAttr, $user['dn']), - 'name' => $this->getUserResponseProperty($user, $displayNameAttr, $userCn), - 'dn' => $user['dn'], + 'uid' => $this->getUserResponseProperty($user, $idAttr, $user['dn']), + 'name' => $this->getUserResponseProperty($user, $displayNameAttr, $userCn), + 'dn' => $user['dn'], 'email' => $this->getUserResponseProperty($user, $emailAttr, null), ]; @@ -187,6 +187,12 @@ class LdapService extends ExternalAuthService throw new LdapException(trans('errors.ldap_extension_not_installed')); } + // Disable certificate verification. + // This option works globally and must be set before a connection is created. + if ($this->config['tls_insecure']) { + $this->ldap->setOption(null, LDAP_OPT_X_TLS_REQUIRE_CERT, LDAP_OPT_X_TLS_NEVER); + } + $serverDetails = $this->parseServerString($this->config['server']); $ldapConnection = $this->ldap->connect($serverDetails['host'], $serverDetails['port']); @@ -198,21 +204,14 @@ class LdapService extends ExternalAuthService if ($this->config['version']) { $this->ldap->setVersion($ldapConnection, $this->config['version']); } - - // Check if TLS_INSECURE is set. The handle is set to NULL due to the nature of - // the LDAP_OPT_X_TLS_REQUIRE_CERT option. It can only be set globally and not per handle. - if ($this->config['tls_insecure']) { - // Setting also ignored? - $this->ldap->setOption($ldapConnection, LDAP_OPT_X_TLS_REQUIRE_CERT, LDAP_OPT_X_TLS_NEVER); - } - if ($this->config['start_tls']) { - // "&& $this->config['tls_cacertfile']" this condition was removed because: - // This ldap option is totally ignored by PHP. (bug: https://bugs.php.net/bug.php?id=73558) - // If we set 'TLS_CACERT /config/ca/bundle.pem' into /etc/openldap/ldap.conf and restart php, ldap_start_tls works. - //$this->ldap->setOption($ldapConnection, LDAP_OPT_X_TLS_CACERTFILE, $this->config['tls_cacertfile']); - ldap_start_tls($ldapConnection); - } + // Start and verify TLS if it's enabled + if ($this->config['start_tls']) { + $started = $this->ldap->startTls($ldapConnection); + if (!$started) { + throw new LdapException('Could not start TLS connection'); + } + } $this->ldapConnection = $ldapConnection; return $this->ldapConnection; diff --git a/phpunit.xml b/phpunit.xml index 033218288..fc1a591a9 100644 --- a/phpunit.xml +++ b/phpunit.xml @@ -36,6 +36,7 @@ + diff --git a/tests/Auth/LdapTest.php b/tests/Auth/LdapTest.php index 3cb39ca2c..840dfd630 100644 --- a/tests/Auth/LdapTest.php +++ b/tests/Auth/LdapTest.php @@ -4,6 +4,7 @@ use BookStack\Auth\Access\LdapService; use BookStack\Auth\Role; use BookStack\Auth\Access\Ldap; use BookStack\Auth\User; +use BookStack\Exceptions\LdapException; use Mockery\MockInterface; use Tests\BrowserKitTest; @@ -40,6 +41,14 @@ class LdapTest extends BrowserKitTest $this->mockUser = factory(User::class)->make(); } + protected function runFailedAuthLogin() + { + $this->commonLdapMocks(1, 1, 1, 1, 1); + $this->mockLdap->shouldReceive('searchAndGetEntries')->times(1) + ->andReturn(['count' => 0]); + $this->post('/login', ['username' => 'timmyjenkins', 'password' => 'cattreedog']); + } + protected function mockEscapes($times = 1) { $this->mockLdap->shouldReceive('escape')->times($times)->andReturnUsing(function($val) { @@ -550,6 +559,22 @@ class LdapTest extends BrowserKitTest ]); } + public function test_start_tls_called_if_option_set() + { + config()->set(['services.ldap.start_tls' => true]); + $this->mockLdap->shouldReceive('startTls')->once()->andReturn(true); + $this->runFailedAuthLogin(); + } + + public function test_connection_fails_if_tls_fails() + { + config()->set(['services.ldap.start_tls' => true]); + $this->mockLdap->shouldReceive('startTls')->once()->andReturn(false); + $this->commonLdapMocks(1, 1, 0, 0, 0); + $this->post('/login', ['username' => 'timmyjenkins', 'password' => 'cattreedog']); + $this->assertResponseStatus(500); + } + public function test_ldap_attributes_can_be_binary_decoded_if_marked() { config()->set(['services.ldap.id_attribute' => 'BIN;uid']); @@ -640,12 +665,7 @@ class LdapTest extends BrowserKitTest { $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->runFailedAuthLogin(); $this->assertTrue($log->hasWarningThatContains('Failed login for timmyjenkins')); } }