-
-
Notifications
You must be signed in to change notification settings - Fork 513
PHPORM-13 Feature Queryable Encryption #2779
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.
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
Conversation
@@ -2176,6 +2183,15 @@ public function isView(): bool | |||
return $this->isView; | |||
} | |||
|
|||
public function isDocument(): bool |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note to leverage this new method in the SchemaManager.
if ($class->isMappedSuperclass || $class->isEmbeddedDocument || $class->isQueryResultDocument || $class->isView()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you intend to make this change in the current PR, or create a ticket to do so later? No objections to doing it now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it's that simple. There are details to work out with File-type collections, which are sometimes Documents and sometimes not. I'll keep that for later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Requesting a Copilot for additional review since I already reviewed a number of these PRs and authored some of them, but LGTM so far.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with phpstan errors fixed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR implements Automatic Queryable Encryption support for the MongoDB ODM, enabling client-side encryption of document fields with the ability to perform queries on encrypted data while maintaining security. The feature requires MongoDB Enterprise 8.0+ or MongoDB Atlas.
Key changes include:
- Added comprehensive queryable encryption mapping support with new
#[Encrypt]
attribute andEncryptQuery
enum - Enhanced configuration to support KMS providers and auto-encryption options
- Extended schema management to automatically create encrypted collections with proper metadata
Reviewed Changes
Copilot reviewed 36 out of 36 changed files in this pull request and generated 1 comment.
Show a summary per file
File | Description |
---|---|
lib/Doctrine/ODM/MongoDB/Mapping/Annotations/Encrypt.php |
New annotation for marking fields/documents as encrypted with query type configuration |
lib/Doctrine/ODM/MongoDB/Mapping/Annotations/EncryptQuery.php |
Enum defining supported query types (Equality, Range) for encrypted fields |
lib/Doctrine/ODM/MongoDB/Configuration.php |
Added KMS provider configuration and auto-encryption options support |
lib/Doctrine/ODM/MongoDB/Utility/EncryptedFieldsMapGenerator.php |
New utility for generating MongoDB encrypted fields maps from ODM metadata |
tests/Documents/Encryption/*.php |
Test document classes demonstrating various encryption scenarios |
tests/Doctrine/ODM/MongoDB/Tests/Functional/QueryableEncryptionTest.php |
Functional tests for encrypted document operations |
docs/en/cookbook/queryable-encryption.rst |
Comprehensive documentation and tutorial for using queryable encryption |
* Add test document classes from tutorial https://github.com/mongodb/docs/blob/master/source/includes/qe-tutorials/node/queryable-encryption-tutorial.js * Create encryption field map * Create encrypted collection * Use promoted properties for encrypted document tests * Revert changes in BaseTestCase * Rename EncryptionFieldMapTest * Create Configuration::getDriverOptions() to create the client * Support class-level #[Encrypt] attribute * Skip QE tests on non-supported server configuration * Add documentation on #[Encrypt] attribute * Add XML mapping for "encrypt" tag * Create specific xsd type for embedded documents to enable <encrypt> tag * Fix import class Throwable * Fix access to $version property before initialization * Use an enum for query type * Make getClientEncryption internal * Improve type of min/max bounds for range queries * Use getWriteOptions with createEncryptedCollection * Use random local master key * Add assertion on non-decrypted data * Ignore all DOCTRINE_MONGODB_DATABASE phpstan errors * Baseline phpstan * Fix CS and skip phpstan issues
The driver already detects encrypted collections
…ction inheritance (#2790)
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
@@ -2176,6 +2183,15 @@ public function isView(): bool | |||
return $this->isView; | |||
} | |||
|
|||
public function isDocument(): bool |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you intend to make this change in the current PR, or create a ticket to do so later? No objections to doing it now.
'bsonType' => match ($mapping['type']) { | ||
ClassMetadata::ONE, Type::HASH => 'object', | ||
ClassMetadata::MANY, Type::COLLECTION => 'array', | ||
Type::INT, Type::INTEGER => 'int', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The BSON type mappings for encryption differentiates between 32-bit and 64-bit integers (int
and long
, respectively). This could be problematic, as the match here is going to limit all integer-mapped fields to 32-bit. Defaulting to 64-bit isn't necessarily a workaround, as the server is going to expect matching types for any equality queries.
I believe this is being tracked by SERVER-75897.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, do you suggest we should change it to long
?
Type::INT, Type::INTEGER => 'int', | |
Type::INT, Type::INTEGER => 'long', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to the CSFLE Encryption Schemas: encrypt
docs, we can specify an array of types for bsonType
. Can we use ['int', 'long']
here to remain flexible?
I'll also share this question in the CSFLE Slack for a sanity check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kevinAlbs replied in Slack:
Notably: though CSFLE schema (JSON Schema) supports multiple BSON types, a QE schema (
encryptedFields
) only supports a single valued bsonType.
So I don't think we have a user-friendly solution here unless we prefer "long" (since it can contain any integer value) and have ODM always encodes the encrypted field as a MongoDB\BSON\Int64
to ensure even values in the 32-bit range are stored as int64 and thus satisfy the QE bsonType
restriction.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When sending a int64 to a field encoded with the int type, we get this error:
mongocryptd error: Cannot encrypt element of type long because schema requires that type is one of: [ int ]
Also, I cannot set multiple types:
mongocryptd error: BSON field 'create.encryptedFields.fields.bsonType' is the wrong type 'array', expected type 'string'
If I change the bson type to long
, then it doesn't accept an int
.
It seems that its a limitation until we add the Int64
type in the ODM.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
New Int64 type: #2805
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, that was a fun little detour
Consider this resolved but I'd like to leave the thread unresolved for posterity (and future accessibility).
lib/Doctrine/ODM/MongoDB/Utility/EncryptedFieldsMapGenerator.php
Outdated
Show resolved
Hide resolved
lib/Doctrine/ODM/MongoDB/Utility/EncryptedFieldsMapGenerator.php
Outdated
Show resolved
Hide resolved
d3049fd
to
492c723
Compare
lib/Doctrine/ODM/MongoDB/Utility/EncryptedFieldsMapGenerator.php
Outdated
Show resolved
Hide resolved
lib/Doctrine/ODM/MongoDB/Utility/EncryptedFieldsMapGenerator.php
Outdated
Show resolved
Hide resolved
tests/Doctrine/ODM/MongoDB/Tests/Tools/EncryptedFieldsMapGeneratorTest.php
Outdated
Show resolved
Hide resolved
], | ||
[ | ||
'path' => 'clientCards', | ||
'bsonType' => 'array', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noted that the EmbedMany relation on Client isn't mapped as encrypted. The objects within the array are encrypted. Would ODM produce the same EncryptedFields object if the relationship on Client was annotated with #[Encrypt]
?
I'll note that it doesn't seem possible to map all array elements as encrypted (based on server manual docs). I suppose one could specify each array index as a separate path, but that wouldn't be flexible for dynamic arrays.
Therefore, I'm not suggesting the current behavior to encrypt the entire array is incorrect. But I am curious about whether users have two ways to define a mapping for the same effect.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The clientCards
field is encrypted as a whole, into a single Binary
value.
QE doesn't allow querying objects, so having an array of documents would not be useful for querying. It it could for partial updates, like pushing a new element.
Meanwhile, we could have a list of int/long/double/date that could be queryable. But this results in this exception:
MongoDB\Driver\Exception\RuntimeException: mongocryptd error: Cannot encrypt element of type: array
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
QE doesn't allow querying objects, so having an array of documents would not be useful for querying. It it could for partial updates, like pushing a new element.
Would we expect this to conflict with ODM's collection strategies? I expect they could end up pushing or pulling elements from the array.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, since arrays are stored as a single binary value, they need to be encrypted and saved globally client-side.
mongocryptd error: $addToSet not allowed on encrypted values
mongocryptd error: $push not allowed on encrypted values
I added the restriction to the documentation.
* Add Int64 type * Convert min/max options in the metadata class * Add type assertions on Type tests
b415d41
to
5746309
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few comments on docs but I think you can address those yourself.
* | ||
* @param array{ keyVaultClient?: Client|Manager, keyVaultNamespace?: string, tlsOptions?: array<string, mixed>, schemaMap?: array<string, mixed>, bypassAutoEncryption?: bool, bypassQueryAnalysis?: bool, encryptedFieldsMap?: array<string,mixed>, extraOptions?: array<string, mixed>} $options | ||
*/ | ||
public function setAutoEncryption(array $options): void |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on the logic in getAutoEncryptionOptions()
below, which merged this in using the spread operator (...
), keys here may overwrite the kmsProviders
and keyVaultNamespace
options created by ODM. On the off chance that could lead to unexpected behavior (or a hypothetical security issue down the line), it may be prudent to document that these options will take precedence.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think there could be a problem:
- In
setAutoEncryption
,kmsProvider
is not allowed to be set (exception). This option is not in the array shape of the param. - In
getAutoEncryptionOptions
,keyVaultNamespace
uses the defaultdb.datakeys
but can be overridden by the value set withsetAutoEncryption
.
Co-authored-by: Jeremy Mikola <[email protected]>
Add Automatic Queryable Encryption to the MongoDB Documents. This feature requires a MongoDB Enterprise license or a MongoDB Atlas cluster.
getDefaultKmsProvider
method name #2784encryptedFields
option when dropping collection #2788Bundle integration: doctrine/DoctrineMongoDBBundle#897