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.

diff --git a/.env.example.complete b/.env.example.complete
index 3f1a8ab9476a9ae71c66ef7939c743e130c472a3..cfc968192760dd075d79f05e3f07f478c6efb80a 100644 (file)
--- 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 56e7aba04f7db005564799578a587fd6d0eff460..e0d7f2615b3ef248d6163f3ae2a0df10406a8cbd 100644 (file)
--- 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 a407b5dc876c7447ca772e7ad26fc11bae7d2f4c..f93ca44f6e768f2a7ed85de2aaaa0ad39a3f4439 100644 (file)
--- 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 34900ce6f70a1d7356f6f45d822a207c73e48870..717167729f61d4991b347546329cb3a1758f930f 100644 (file)
--- 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();
+    }
 }