From 8087123f2e1823d6584844a540bf4639c55f4fad Mon Sep 17 00:00:00 2001 From: Dan Brown Date: Thu, 2 May 2024 22:56:51 +0100 Subject: [PATCH] LDAP: Review, testing and update of LDAP TLS CA cert control Review of #4913 Added testing to cover option. Updated option so it can be used for a CA directory, or a CA file. Updated option name to be somewhat abstracted from original underling PHP option. Tested against Jumpcloud. Testing took hours due to instability which was due to these settings sticking and being unstable on change until php process restart. Also due to little documentation for these options. X_TLS_CACERTDIR option needs cert files to be named via specific hashes which can be achieved via c_rehash utility. This also adds detail on STARTTLS failure, which took a long time to discover due to little detail out there for deeper PHP LDAP debugging. --- .env.example.complete | 2 +- app/Access/LdapService.php | 42 ++++++++++++++++++++++++++++++++++---- app/Config/services.php | 2 +- tests/Auth/LdapTest.php | 32 +++++++++++++++++++++++++++++ 4 files changed, 72 insertions(+), 6 deletions(-) diff --git a/.env.example.complete b/.env.example.complete index 3f1a8ab94..cfc968192 100644 --- a/.env.example.complete +++ b/.env.example.complete @@ -219,7 +219,7 @@ LDAP_USER_FILTER=false LDAP_VERSION=false LDAP_START_TLS=false LDAP_TLS_INSECURE=false -LDAP_TLS_CACERTFILE=false +LDAP_TLS_CA_CERT=false LDAP_ID_ATTRIBUTE=uid LDAP_EMAIL_ATTRIBUTE=mail LDAP_DISPLAY_NAME_ATTRIBUTE=cn diff --git a/app/Access/LdapService.php b/app/Access/LdapService.php index 56e7aba04..e0d7f2615 100644 --- a/app/Access/LdapService.php +++ b/app/Access/LdapService.php @@ -209,10 +209,10 @@ class LdapService $this->ldap->setOption(null, LDAP_OPT_X_TLS_REQUIRE_CERT, LDAP_OPT_X_TLS_NEVER); } - // Specify CA Cert file for LDAP. + // Configure any user-provided CA cert files for LDAP. // This option works globally and must be set before a connection is created. - if ($this->config['tls_cacertfile']) { - $this->ldap->setOption(null, LDAP_OPT_X_TLS_CACERTFILE, $this->config['tls_cacertfile']); + if ($this->config['tls_ca_cert']) { + $this->configureTlsCaCerts($this->config['tls_ca_cert']); } $ldapHost = $this->parseServerString($this->config['server']); @@ -229,7 +229,14 @@ class LdapService // Start and verify TLS if it's enabled if ($this->config['start_tls']) { - $started = $this->ldap->startTls($ldapConnection); + try { + $started = $this->ldap->startTls($ldapConnection); + } catch (\Exception $exception) { + $error = $exception->getMessage() . ' :: ' . ldap_error($ldapConnection); + ldap_get_option($ldapConnection, LDAP_OPT_DIAGNOSTIC_MESSAGE, $detail); + Log::info("LDAP STARTTLS failure: {$error} {$detail}"); + throw new LdapException('Could not start TLS connection. Further details in the application log.'); + } if (!$started) { throw new LdapException('Could not start TLS connection'); } @@ -240,6 +247,33 @@ class LdapService return $this->ldapConnection; } + /** + * Configure TLS CA certs globally for ldap use. + * This will detect if the given path is a directory or file, and set the relevant + * LDAP TLS options appropriately otherwise throw an exception if no file/folder found. + * + * Note: When using a folder, certificates are expected to be correctly named by hash + * which can be done via the c_rehash utility. + * + * @throws LdapException + */ + protected function configureTlsCaCerts(string $caCertPath): void + { + $errMessage = "Provided path [{$caCertPath}] for LDAP TLS CA certs could not be resolved to an existing location"; + $path = realpath($caCertPath); + if ($path === false) { + throw new LdapException($errMessage); + } + + if (is_dir($path)) { + $this->ldap->setOption(null, LDAP_OPT_X_TLS_CACERTDIR, $path); + } else if (is_file($path)) { + $this->ldap->setOption(null, LDAP_OPT_X_TLS_CACERTFILE, $path); + } else { + throw new LdapException($errMessage); + } + } + /** * Parse an LDAP server string and return the host suitable for a connection. * Is flexible to formats such as 'ldap.example.com:8069' or 'ldaps://ldap.example.com'. diff --git a/app/Config/services.php b/app/Config/services.php index a407b5dc8..f93ca44f6 100644 --- a/app/Config/services.php +++ b/app/Config/services.php @@ -133,7 +133,7 @@ return [ 'group_attribute' => env('LDAP_GROUP_ATTRIBUTE', 'memberOf'), 'remove_from_groups' => env('LDAP_REMOVE_FROM_GROUPS', false), 'tls_insecure' => env('LDAP_TLS_INSECURE', false), - 'tls_cacertfile' => env('LDAP_TLS_CACERTFILE', false), + 'tls_ca_cert' => env('LDAP_TLS_CA_CERT', false), 'start_tls' => env('LDAP_START_TLS', false), 'thumbnail_attribute' => env('LDAP_THUMBNAIL_ATTRIBUTE', null), ], diff --git a/tests/Auth/LdapTest.php b/tests/Auth/LdapTest.php index 34900ce6f..717167729 100644 --- a/tests/Auth/LdapTest.php +++ b/tests/Auth/LdapTest.php @@ -4,6 +4,7 @@ namespace Tests\Auth; use BookStack\Access\Ldap; use BookStack\Access\LdapService; +use BookStack\Exceptions\LdapException; use BookStack\Users\Models\Role; use BookStack\Users\Models\User; use Illuminate\Testing\TestResponse; @@ -35,6 +36,7 @@ class LdapTest extends TestCase 'services.ldap.user_filter' => '(&(uid=${user}))', 'services.ldap.follow_referrals' => false, 'services.ldap.tls_insecure' => false, + 'services.ldap.tls_ca_cert' => false, 'services.ldap.thumbnail_attribute' => null, ]); $this->mockLdap = $this->mock(Ldap::class); @@ -767,4 +769,34 @@ EBEQCgwSExIQEw8QEBD/yQALCAABAAEBAREA/8wABgAQEAX/2gAIAQEAAD8A0s8g/9k=')], $this->assertNotNull($user->avatar); $this->assertEquals('8c90748342f19b195b9c6b4eff742ded', md5_file(public_path($user->avatar->path))); } + + public function test_tls_ca_cert_option_throws_if_set_to_invalid_location() + { + $path = 'non_found_' . time(); + config()->set(['services.ldap.tls_ca_cert' => $path]); + + $this->commonLdapMocks(0, 0, 0, 0, 0); + + $this->assertThrows(function () { + $this->withoutExceptionHandling()->mockUserLogin(); + }, LdapException::class, "Provided path [{$path}] for LDAP TLS CA certs could not be resolved to an existing location"); + } + + public function test_tls_ca_cert_option_used_if_set_to_a_folder() + { + $path = $this->files->testFilePath(''); + config()->set(['services.ldap.tls_ca_cert' => $path]); + + $this->mockLdap->shouldReceive('setOption')->once()->with(null, LDAP_OPT_X_TLS_CACERTDIR, rtrim($path, '/'))->andReturn(true); + $this->runFailedAuthLogin(); + } + + public function test_tls_ca_cert_option_used_if_set_to_a_file() + { + $path = $this->files->testFilePath('test-file.txt'); + config()->set(['services.ldap.tls_ca_cert' => $path]); + + $this->mockLdap->shouldReceive('setOption')->once()->with(null, LDAP_OPT_X_TLS_CACERTFILE, $path)->andReturn(true); + $this->runFailedAuthLogin(); + } }