[Encryption] Improve diagnostic command #898

GromNaN · 2025-06-18T09:31:59Z

This improves the output of the diagnostics command and adds a command that prints the encrypted fields map for use in configuration.

Output:

❯ bin/console doctrine:mongodb:connection:diagnostic 

MongoDB Encryption Diagnostics
==============================

 PHP Environment
 * MongoDB extension loaded: Yes
 * MongoDB extension version: 1.21.1
 * MongoDB extension supports libmongocrypt: Yes
 * MongoDB library version: 2.1.0

 mongocryptd information
 * mongocryptd path: /Users/jerome/.local/bin/mongocryptd
 * mongocryptd version: mongocrypt version v8.0.8

Connection: default
-------------------

 Server Information
 * Server Version: 8.0.8
 * Topology: Replica Set

 Auto Encryption Configuration
 * Auto Encryption Enabled: Yes
 * Key Vault Namespace: symfony.datakeys
 * Key Count: 0

                                                                                                         
 [OK] System looks ok for encryption support.

In case of incorrect setup:

MongoDB Encryption Diagnostics
==============================

 PHP Environment
 * MongoDB extension loaded: Yes
 * MongoDB extension version: 2.1.0
 * MongoDB extension supports libmongocrypt: Yes
 * MongoDB library version: 2.1.0

 mongocryptd information
 * mongocryptd: not found

Connection: default
-------------------

 Server Information
 * Server Version: 8.0.8
 * Topology: Standalone

                                                                                                         
 [WARNING] This topology does not support encryption.                                                    
                                                                                                         

 Auto Encryption Configuration

                                                                                                         
 [ERROR] Failed to retrieve auto encryption information: mongocryptd error: No suitable servers found:   
         `serverSelectionTimeoutMS` expired: [connection refused calling hello on 'localhost:27020'].    
         Topology type: Single:                                                                          
                                                                                                         

Connection: other
-----------------

 Server Information
 * Server Version: 8.0.8
 * Topology: Standalone

                                                                                                         
 [WARNING] This topology does not support encryption.                                                    
                                                                                                         

 Auto Encryption Configuration
 Auto encryption is not enabled for this connection.

                                                                                                         
 [WARNING] Not all requirements for encryption support are met. Please check the diagnostics above.

GromNaN · 2025-06-19T21:07:47Z

src/Command/ConnectionDiagnosticCommand.php

-            } catch (Throwable $exception) {
-                $io->error('Could not retrieve auto encryption info: ' . $exception->getMessage());
-            }
+            $configOk   = $this->printAndCheckConnectionDiagnostic($name, $diagnostic, $io) && $configOk;


When multiple connections are configured, but they don't have all auto encryption enabled, then the conclusion says there is an issue. We should return "OK" when auto encryption is not configured on a connection?

Ah, that's a good point. Indeed, if encryption wasn't configured on a connection, we should print an info line and mark it as "ok".

commit b494ea1 Author: Andreas Braun <[email protected]> Date: Thu Jun 12 13:36:37 2025 +0200 Add command to dump encrypted fields map commit a74e799 Author: Andreas Braun <[email protected]> Date: Thu Jun 12 13:10:11 2025 +0200 Extract connection diagnostics into separate print method commit 69d205d Author: Andreas Braun <[email protected]> Date: Wed Jun 11 14:58:49 2025 +0200 Extract encryption diagnostic to separate class commit 1c1b2e2 Author: Andreas Braun <[email protected]> Date: Wed Jun 11 14:41:50 2025 +0200 Improve connection diagnostics output commit f19bc06 Merge: e24dd48 e09b2a2 Author: Jérôme Tamarelle <[email protected]> Date: Thu Jun 12 10:04:33 2025 +0200 Merge remote-tracking branch 'upstream/5.4.x' into fix-kms-xsd-docs-alignment commit e24dd48 Author: Jérôme Tamarelle <[email protected]> Date: Wed Jun 11 11:36:16 2025 +0200 Add AutoEncryptionInfo to the diagnostic command commit ea7b50f Author: Jérôme Tamarelle <[email protected]> Date: Tue May 27 18:24:46 2025 +0200 Introduce connection:diagnotic command commit 60cc9c2 Author: Jérôme Tamarelle <[email protected]> Date: Mon May 26 18:21:32 2025 +0000 Add autoEncryption configuration to the client Improve connection diagnostics output

GromNaN · 2025-06-23T07:14:24Z

src/DependencyInjection/DoctrineMongoDBExtension.php

+            if (isset($driverOptions['autoEncryption']['extraOptions']['cryptSharedLibPath'])) {
+                $fs                 = new Filesystem();
+                $cryptSharedLibPath = $driverOptions['autoEncryption']['extraOptions']['cryptSharedLibPath'];
+
+                // If it's not an absolute path, resolve it relative to the project root
+                if (! $fs->isAbsolutePath($cryptSharedLibPath)) {
+                    $driverOptions['autoEncryption']['extraOptions']['cryptSharedLibPath'] = realpath($cryptSharedLibPath);
+                }
+            }


@alcaeus I'm reverting this part. In Symfony we never convert relative paths to absolute.

I'll add documentation using %kernel.project_dir% instead.

Sounds good - I wasn't quite sure if the kernel parameter was resolved properly. If that works, I prefer that over changing the path in the extension.

GromNaN · 2025-06-23T07:17:36Z

src/Command/ConnectionDiagnosticCommand.php

-    {
+    public function __construct(
+        private readonly ServiceProviderInterface $diagnostics,
+        private readonly EncryptionDiagnostic $encryptionDiagnostic = new EncryptionDiagnostic(),


We don't declare a service for EncryptionDiagnostic. Only leverage the default value from the constructor.

docs/encryption.rst

paulinevos · 2025-06-26T13:24:26Z

There's a lot of commits in this. Does it need to be rebased?

GromNaN · 2025-06-26T13:28:35Z

There's a lot of commits in this. Does it need to be rebased?

It will be squashed on merge. All the commits represent the iterations on this PR.

paulinevos

some small comments, otherwise seems fine

paulinevos · 2025-06-26T13:38:23Z

src/Command/ConnectionDiagnosticCommand.php

+                'Key Vault Namespace: ' . $autoEncryptionInfo['keyVaultNamespace'],
+                'Key Count: ' . $autoEncryptionInfo['keyCount'],
+            ]);
+        } catch (RuntimeException $e) {


Why not catch something broader? I'm guessing there could be other exception types that we'd want to catch here?

I only want to catch driver errors due to the command that is run. I don't expect other kind of exception to occurs, so if there is an other error I don't want to catch it, but I want the regular error handler to display it (with the backtrace).

paulinevos · 2025-06-26T13:45:28Z

docs/encryption.rst

-Queryable Encryption (QE) allows you to run queries on encrypted fields. To use QE, you may need to provide an ``encryptedFieldsMap`` or use a schema map, depending on your driver and use case.
+You can configure which fields are encrypted in each collection by specifying the
+``autoEncryption.encryptedFieldsMap`` option in the connection configuration.
+This setting **recommended** for improved security and performance.


Missing is after this setting

Suggested change

This setting **recommended** for improved security and performance.

This setting is **recommended** for improved security and performance.

paulinevos · 2025-06-26T13:47:23Z

src/DependencyInjection/DoctrineMongoDBExtension.php

@@ -44,7 +44,6 @@
 use Symfony\Component\DependencyInjection\Loader\PhpFileLoader;
 use Symfony\Component\DependencyInjection\Reference;
 use Symfony\Component\ExpressionLanguage\ExpressionLanguage;
-use Symfony\Component\Filesystem\Filesystem;


I would use fixup to add this commit to either:

whichever commit introduced the CS errors if that commit is part of this PR, or

fix it up to the other fix CS commit you already have

Not a big deal not but makes reviewing a little easier to go commit by commit

paulinevos · 2025-06-26T13:49:00Z

src/Command/ConnectionDiagnosticCommand.php

    private function printAndCheckConnectionDiagnostic(string $name, ConnectionDiagnostic $diagnostic, SymfonyStyle $io): bool
    {
        $io->section(sprintf('Connection: %s', $name));

-        $configOk = $this->printAndCheckServerInfo($io, $diagnostic);
-        $this->printAutoEncryptionConfiguration($io, $diagnostic);
+        $enabled = $this->printAutoEncryptionConfiguration($io, $diagnostic);


Maybe rename this to autoEncryptionEnabled?

GromNaN changed the base branch from 5.4.x to feature/queryable-encryption June 18, 2025 09:32

GromNaN mentioned this pull request Jun 19, 2025

Improve diagnostic command GromNaN/DoctrineMongoDBBundle#1

Closed

GromNaN force-pushed the improve-diagnostic-command branch from f75dee1 to 30a90d3 Compare June 19, 2025 20:17

GromNaN requested a review from alcaeus June 19, 2025 20:56

GromNaN marked this pull request as ready for review June 19, 2025 20:56

GromNaN commented Jun 19, 2025

View reviewed changes

GromNaN force-pushed the improve-diagnostic-command branch from 9f8a963 to b4d1412 Compare June 19, 2025 21:37

GromNaN mentioned this pull request Jun 19, 2025

Enable auto encryption #897

Draft

5 tasks

alcaeus and others added 14 commits June 23, 2025 09:00

Extract encryption diagnostic to separate class

4b4e5e6

Extract connection diagnostics into separate print method

4ac845b

Add command to dump encrypted fields map

509c2f4

Fix CS and static analysis

cf0d366

Fix DIC and mark diagnostic classes as internal

353bb5a

Run CI

48ce7b0

Catch runtime exception when mongocryptd is not running

f97c1c1

Always print diagnostic for every connection, even if one is not OK

9e5416f

Remove duplicate service definition for diagnostic per connection

7e9a2d3

Add test to diagnostic command

2e58e41

Add json format to the dump command

3065c46

Improve encrypted fields map docs

627fe52

Fix CS

5c3e66d

GromNaN commented Jun 23, 2025

View reviewed changes

GromNaN added 2 commits June 23, 2025 09:28

Skip server check when auto encryption is not enabled

32697ff

Add cryptSharedLibPath docs

e192526

GromNaN force-pushed the improve-diagnostic-command branch from b4d1412 to e192526 Compare June 23, 2025 08:11

Update test to not dependents on mongocryptd availability

04075c6

alcaeus reviewed Jun 23, 2025

View reviewed changes

docs/encryption.rst Outdated Show resolved Hide resolved

GromNaN force-pushed the improve-diagnostic-command branch from 381dfba to 6b300d7 Compare June 23, 2025 13:50

Fix mongo_crypt_v1.so name

cd2a184

GromNaN force-pushed the improve-diagnostic-command branch from 6b300d7 to cd2a184 Compare June 23, 2025 13:51

alcaeus requested a review from paulinevos June 25, 2025 15:23

paulinevos requested changes Jun 26, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[Encryption] Improve diagnostic command #898

[Encryption] Improve diagnostic command #898

Uh oh!

GromNaN commented Jun 18, 2025 •

edited

Loading

Uh oh!

GromNaN Jun 19, 2025

Uh oh!

alcaeus Jun 23, 2025

Uh oh!

GromNaN Jun 23, 2025

Uh oh!

alcaeus Jun 23, 2025

Uh oh!

GromNaN Jun 23, 2025

Uh oh!

Uh oh!

paulinevos commented Jun 26, 2025

Uh oh!

GromNaN commented Jun 26, 2025

Uh oh!

paulinevos left a comment

Uh oh!

paulinevos Jun 26, 2025

Uh oh!

GromNaN Jun 26, 2025

Uh oh!

paulinevos Jun 26, 2025

Uh oh!

GromNaN Jun 26, 2025

Uh oh!

paulinevos Jun 26, 2025

Uh oh!

paulinevos Jun 26, 2025

Uh oh!

Uh oh!

	This setting recommended for improved security and performance.
	This setting is recommended for improved security and performance.

[Encryption] Improve diagnostic command #898

Are you sure you want to change the base?

[Encryption] Improve diagnostic command #898

Uh oh!

Conversation

GromNaN commented Jun 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

paulinevos commented Jun 26, 2025

Uh oh!

GromNaN commented Jun 26, 2025

Uh oh!

paulinevos left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

GromNaN commented Jun 18, 2025 •

edited

Loading