Skip to content

PHPLIB-892: Require crypt_shared and/or mongocryptd for tests utilizing enterprise CSFLE features #1072

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Jun 26, 2023
Merged

PHPLIB-892: Require crypt_shared and/or mongocryptd for tests utilizing enterprise CSFLE features #1072

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
35c7bbe
Skip CSFLE automatic encryption tests for non-enterprise servers
GromNaN May 17, 2023
ae8b61c
Refactor isCryptSharedLibAvailable & isMongocryptdAvailable in the ba…
GromNaN Jun 20, 2023
5d8be5d
remove incorrect comment
GromNaN Jun 21, 2023
96d27b7
Checks for crypt_shared and/or mongocryptd in FunctionalTestCase::ski…
GromNaN Jun 26, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 1 addition & 4 deletions tests/DocumentationExamplesTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1810,10 +1810,7 @@ public function testQueryableEncryption(): void
}

$this->skipIfServerVersion('<', '7.0.0', 'Explicit encryption tests require MongoDB 7.0 or later');

if (! $this->isEnterprise()) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In PHPLIB-892 you wrote:

I'll run the DocumentationExamplesTest::testQueryableEncryption() with crypt_shared and/or mongocryptd available with the community server. If it passes, let's remove the isEnterprise() method and change the logic in DocumentationExamplesTest::testQueryableEncryption() to instead check if either crypt_shared or mongocryptd is available.

skipIfClientSideEncryptionIsNotSupported() doesn't check for either of those things. Did something change since you wrote that comment?

Also, in an earlier comment on that issue I pointed out that DocumentationExamplesTest::testQueryableEncryption() did require crypt_shared and/or mongocryptd (since it uses queryable encryption) but the prose test did not (since it explicitly encrypts). You could run the prose tests w/o crypt_shared and mongocryptd to verify that.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@GromNaN: In light of mongodb/specifications#1439, I would be in favor of adding a check for crypt_shared and/or mongocryptd to the base skipIfClientSideEncryptionIsNotSupported() method.

A call to skipIfClientSideEncryptionIsNotSupported() could then be used here (after the topology and 7.0+ version check). I'm aware the version check in skipIfClientSideEncryptionIsNotSupported() would be redundant, but that's OK.

You could then also remove the explicit crypt_shared/mongocryptd check in ClientSideEncryptionSpecTest::setUp().

With that done, skipIfClientSideEncryptionIsNotSupported() would become consistent with UnifiedTestRunner::isClientSideEncryptionSupported() and all of our tests would perform the same checks.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm aware the version check in skipIfClientSideEncryptionIsNotSupported() would be redundant, but that's OK.

There is a slight difference between the server version check and the feature compatibility version check in skipIfClientSideEncryptionIsNotSupported. So this is not redundant.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I followed your instructions.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aye, technically different checks. I said "redundant" as our test suite is never run on mixed-version or upgraded replica sets, so FCV will always equate the server version.

$this->markTestSkipped('Automatic encryption requires MongoDB Enterprise');
}
$this->skipIfClientSideEncryptionIsNotSupported();

// Fetch names for the database and collection under test
$collectionName = $this->getCollectionName();
Expand Down
49 changes: 34 additions & 15 deletions tests/FunctionalTestCase.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,10 +28,11 @@
use function filter_var;
use function getenv;
use function implode;
use function in_array;
use function is_array;
use function is_callable;
use function is_executable;
use function is_object;
use function is_readable;
use function is_string;
use function key;
use function ob_get_clean;
Expand All @@ -44,8 +45,10 @@
use function sprintf;
use function version_compare;

use const DIRECTORY_SEPARATOR;
use const FILTER_VALIDATE_BOOLEAN;
use const INFO_MODULES;
use const PATH_SEPARATOR;

abstract class FunctionalTestCase extends TestCase
{
Expand Down Expand Up @@ -397,20 +400,6 @@ protected function isApiVersionRequired(): bool
return isset($document->requireApiVersion) && $document->requireApiVersion === true;
}

protected function isEnterprise(): bool
{
$buildInfo = $this->getPrimaryServer()->executeCommand(
$this->getDatabaseName(),
new Command(['buildInfo' => 1])
)->toArray()[0];

if (isset($buildInfo->modules) && is_array($buildInfo->modules)) {
return in_array('enterprise', $buildInfo->modules);
}

throw new UnexpectedValueException('Could not determine server modules');
}

protected function isLoadBalanced()
{
return $this->getPrimaryServer()->getType() == Server::TYPE_LOAD_BALANCER;
Expand Down Expand Up @@ -542,6 +531,10 @@ protected function skipIfClientSideEncryptionIsNotSupported(): void
if (static::getModuleInfo('libmongocrypt') === 'disabled') {
$this->markTestSkipped('Client Side Encryption is not enabled in the MongoDB extension');
}

if (! static::isCryptSharedLibAvailable() && ! static::isMongocryptdAvailable()) {
$this->markTestSkipped('Neither crypt_shared nor mongocryptd are available');
}
}

protected function skipIfGeoHaystackIndexIsNotSupported(): void
Expand Down Expand Up @@ -578,6 +571,32 @@ protected function skipIfTransactionsAreNotSupported(): void
}
}

/** @see https://www.mongodb.com/docs/manual/core/queryable-encryption/reference/shared-library/ */
public static function isCryptSharedLibAvailable(): bool
{
$cryptSharedLibPath = getenv('CRYPT_SHARED_LIB_PATH');

if ($cryptSharedLibPath === false) {
return false;
}

return is_readable($cryptSharedLibPath);
}

/** @see https://www.mongodb.com/docs/manual/core/queryable-encryption/reference/mongocryptd/ */
public static function isMongocryptdAvailable(): bool
{
$paths = explode(PATH_SEPARATOR, getenv("PATH"));

foreach ($paths as $path) {
if (is_executable($path . DIRECTORY_SEPARATOR . 'mongocryptd')) {
return true;
}
}

return false;
}

private static function appendAuthenticationOptions(array $options): array
{
if (isset($options['username']) || isset($options['password'])) {
Expand Down
30 changes: 0 additions & 30 deletions tests/Operation/CreateEncryptedCollectionFunctionalTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,13 +12,7 @@
use MongoDB\Operation\CreateEncryptedCollection;

use function base64_decode;
use function explode;
use function getenv;
use function is_executable;
use function is_readable;

use const DIRECTORY_SEPARATOR;
use const PATH_SEPARATOR;

class CreateEncryptedCollectionFunctionalTest extends FunctionalTestCase
{
Expand Down Expand Up @@ -217,28 +211,4 @@ public static function createTestClient(?string $uri = null, array $options = []

return parent::createTestClient($uri, $options, $driverOptions);
}

private static function isCryptSharedLibAvailable(): bool
{
$cryptSharedLibPath = getenv('CRYPT_SHARED_LIB_PATH');

if ($cryptSharedLibPath === false) {
return false;
}

return is_readable($cryptSharedLibPath);
}

private static function isMongocryptdAvailable(): bool
{
$paths = explode(PATH_SEPARATOR, getenv("PATH"));

foreach ($paths as $path) {
if (is_executable($path . DIRECTORY_SEPARATOR . 'mongocryptd')) {
return true;
}
}

return false;
}
}
30 changes: 0 additions & 30 deletions tests/SpecTests/ClientSideEncryption/FunctionalTestCase.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,15 +8,9 @@
use PHPUnit\Framework\Assert;
use stdClass;

use function explode;
use function getenv;
use function is_executable;
use function is_readable;
use function sprintf;

use const DIRECTORY_SEPARATOR;
use const PATH_SEPARATOR;

/**
* Base class for client-side encryption prose tests.
*
Expand Down Expand Up @@ -92,28 +86,4 @@ private static function getEnv(string $name): string

return $value;
}

private static function isCryptSharedLibAvailable(): bool
{
$cryptSharedLibPath = getenv('CRYPT_SHARED_LIB_PATH');

if ($cryptSharedLibPath === false) {
return false;
}

return is_readable($cryptSharedLibPath);
}

private static function isMongocryptdAvailable(): bool
{
$paths = explode(PATH_SEPARATOR, getenv("PATH"));

foreach ($paths as $path) {
if (is_executable($path . DIRECTORY_SEPARATOR . 'mongocryptd')) {
return true;
}
}

return false;
}
}
34 changes: 0 additions & 34 deletions tests/SpecTests/ClientSideEncryptionSpecTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,22 +32,16 @@
use function base64_decode;
use function basename;
use function count;
use function explode;
use function file_get_contents;
use function getenv;
use function glob;
use function in_array;
use function is_executable;
use function is_readable;
use function iterator_to_array;
use function json_decode;
use function sprintf;
use function str_repeat;
use function substr;

use const DIRECTORY_SEPARATOR;
use const PATH_SEPARATOR;

/**
* Client-side encryption spec tests.
*
Expand Down Expand Up @@ -123,10 +117,6 @@ public function setUp(): void
parent::setUp();

$this->skipIfClientSideEncryptionIsNotSupported();

if (! static::isCryptSharedLibAvailable() && ! static::isMongocryptdAvailable()) {
$this->markTestSkipped('Neither crypt_shared nor mongocryptd are available');
}
}

/**
Expand Down Expand Up @@ -1988,28 +1978,4 @@ private function prepareCorpusData(string $fieldName, stdClass $data, ClientEncr

return $data->allowed ? $returnData : $data;
}

private static function isCryptSharedLibAvailable(): bool
{
$cryptSharedLibPath = getenv('CRYPT_SHARED_LIB_PATH');

if ($cryptSharedLibPath === false) {
return false;
}

return is_readable($cryptSharedLibPath);
}

private static function isMongocryptdAvailable(): bool
{
$paths = explode(PATH_SEPARATOR, getenv("PATH"));

foreach ($paths as $path) {
if (is_executable($path . DIRECTORY_SEPARATOR . 'mongocryptd')) {
return true;
}
}

return false;
}
}
32 changes: 3 additions & 29 deletions tests/UnifiedSpecTests/UnifiedTestRunner.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,6 @@
use function getenv;
use function implode;
use function in_array;
use function is_executable;
use function is_readable;
use function is_string;
use function parse_url;
use function PHPUnit\Framework\assertContainsOnly;
Expand All @@ -42,9 +40,7 @@
use function substr_replace;
use function version_compare;

use const DIRECTORY_SEPARATOR;
use const FILTER_VALIDATE_BOOLEAN;
use const PATH_SEPARATOR;

/**
* Unified test runner.
Expand Down Expand Up @@ -341,6 +337,8 @@ private function isAuthenticated(): bool

/**
* Return whether client-side encryption is supported.
*
* @see FunctionalTestCase::skipIfClientSideEncryptionIsNotSupported()
*/
private function isClientSideEncryptionSupported(): bool
{
Expand All @@ -354,31 +352,7 @@ private function isClientSideEncryptionSupported(): bool
return false;
}

return static::isCryptSharedLibAvailable() || static::isMongocryptdAvailable();
}

private static function isCryptSharedLibAvailable(): bool
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@eramongodb, @kevinAlbs: The csfle runOnRequirement dates back to mongodb/specifications@0f65908 and DRIVERS-2017. Reading the existing language in the unified spec, there's no indication that crypt_shared and/or mongocryptd are required.

If true, the tests MUST only run if the driver and server support Client-Side Field Level Encryption. A server supports CSFLE if it is version 4.2.0 or higher. If false, tests MUST only run if CSFLE is not enabled. If this field is omitted, there is no CSFLE requirement.

The PHPLIB unified test runner currently checks three things:

  • Server FCV >= 4.2
  • PHPC is compiled with libmongocrypt (not just libmongoc)
  • At least one of crypt_shared and mongocryptd is available

I realize that the only unified CSFLE tests at the moment are for key management, which isn't an enterprise feature. But ignoring that for a moment and assuming legacy tests will eventually be ported to the unified format, was it your intention that the csfle requirement also ensure that queryable encryption be supported?

If yes, then I'd like to clarify this in the unified test spec. If no, then I think we should remove the crypt_shared/mongocryptd check from PHPLIB's unified test runner (and maybe still clarify that in the unified spec).

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At least one of crypt_shared and mongocryptd is available

I expect crypt_shared or mongocryptd would be required once the unified test format supports automatic encryption. I expect it is not necessary for the current unified test format. I suggest keeping the check for crypt_shared or mongocryptd, as I expect it will be needed in the future.

was it your intention that the csfle requirement also ensure that queryable encryption be supported?

I think yes. Both "Client-Side Field Level Encryption" and "Queryable Encryption" require libmongocrypt. As of DRIVERS-2454, "In-Use Encryption" can be used to refer to both features.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. I opened mongodb/specifications#1439 and DRIVERS-2661 to update the unified/legacy test docs.

Copy link

@eramongodb eramongodb Jun 26, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The csfle requirement was added as an (overly?) simple all-or-nothing check for whether anything related to Client Side Encryption is required by the test, which implicitly requires - or at least, was assumed to require - either mongocryptd (the only consideration back then) or crypt_shared (a new option since the wording was added), since most CSE functions cannot fulfill their specified behavior without them, including the Key Management API. I support the clarification of wording that documents the mongocryptd/crypt_shared requirement; submitted an approving review for mongodb/specifications#1439 accordingly.

{
$cryptSharedLibPath = getenv('CRYPT_SHARED_LIB_PATH');

if ($cryptSharedLibPath === false) {
return false;
}

return is_readable($cryptSharedLibPath);
}

private static function isMongocryptdAvailable(): bool
{
$paths = explode(PATH_SEPARATOR, getenv("PATH"));

foreach ($paths as $path) {
if (is_executable($path . DIRECTORY_SEPARATOR . 'mongocryptd')) {
return true;
}
}

return false;
return FunctionalTestCase::isCryptSharedLibAvailable() || FunctionalTestCase::isMongocryptdAvailable();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is one thing I had in mind when creating PHPLIB-1170.

}

/**
Expand Down