Skip to content

Commit

Permalink
Merge pull request BookStackApp#4985 from BookStackApp/ldap_ca_cert_c…
Browse files Browse the repository at this point in the history
…ontrol

LDAP CA TLS Cert Option, PR Review and continuation
  • Loading branch information
ssddanbrown authored May 2, 2024
2 parents 4c1c315 + 8087123 commit b072077
Show file tree
Hide file tree
Showing 4 changed files with 75 additions and 1 deletion.
1 change: 1 addition & 0 deletions .env.example.complete
Original file line number Diff line number Diff line change
Expand Up @@ -219,6 +219,7 @@ LDAP_USER_FILTER="(&(uid={user}))"
LDAP_VERSION=false
LDAP_START_TLS=false
LDAP_TLS_INSECURE=false
LDAP_TLS_CA_CERT=false
LDAP_ID_ATTRIBUTE=uid
LDAP_EMAIL_ATTRIBUTE=mail
LDAP_DISPLAY_NAME_ATTRIBUTE=cn
Expand Down
42 changes: 41 additions & 1 deletion app/Access/LdapService.php
Original file line number Diff line number Diff line change
Expand Up @@ -209,6 +209,12 @@ protected function getConnection()
$this->ldap->setOption(null, LDAP_OPT_X_TLS_REQUIRE_CERT, LDAP_OPT_X_TLS_NEVER);
}

// 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_ca_cert']) {
$this->configureTlsCaCerts($this->config['tls_ca_cert']);
}

$ldapHost = $this->parseServerString($this->config['server']);
$ldapConnection = $this->ldap->connect($ldapHost);

Expand All @@ -223,7 +229,14 @@ protected function getConnection()

// 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');
}
Expand All @@ -234,6 +247,33 @@ protected function getConnection()
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'.
Expand Down
1 change: 1 addition & 0 deletions app/Config/services.php
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,7 @@
'group_attribute' => env('LDAP_GROUP_ATTRIBUTE', 'memberOf'),
'remove_from_groups' => env('LDAP_REMOVE_FROM_GROUPS', false),
'tls_insecure' => env('LDAP_TLS_INSECURE', false),
'tls_ca_cert' => env('LDAP_TLS_CA_CERT', false),
'start_tls' => env('LDAP_START_TLS', false),
'thumbnail_attribute' => env('LDAP_THUMBNAIL_ATTRIBUTE', null),
],
Expand Down
32 changes: 32 additions & 0 deletions tests/Auth/LdapTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

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;
Expand Down Expand Up @@ -35,6 +36,7 @@ protected function setUp(): void
'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);
Expand Down Expand Up @@ -799,4 +801,34 @@ public function test_thumbnail_attribute_used_as_user_avatar_if_configured()
$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();
}
}

0 comments on commit b072077

Please sign in to comment.