-
-
Notifications
You must be signed in to change notification settings - Fork 3.9k
CSFLE support #15390
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
CSFLE support #15390
Conversation
…ncrypted-schemas-mongoose-main feat(NODE-6506): Add support for encrypted schemas
feat(NODE-6507): generate encryption configuration on mongoose connect
ci(NODE-6919): Use `mongodb-runner` in encryption cluster setup
feat: expose a ClientEncryption object on encrypted Models
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 adds support for client‐side field level encryption (CSFLE) in Mongoose by introducing autoEncryptionType methods on individual schema types and integrating encryption configuration at both the schema and connection levels.
- Introduces autoEncryptionType methods for SchemaType, SchemaString, SchemaObjectId, and other schema types that return their corresponding BSON type names.
- Updates schema construction (e.g. in lib/schema.js) to include an encryptionType option and maintain an encryptedFields map, and extends schema methods (pick, add) for encrypted fields.
- Adds a Model.clientEncryption helper and updates driver connection creation to build encryption schemas, along with documentation and CI adjustments.
Reviewed Changes
Copilot reviewed 31 out of 31 changed files in this pull request and generated 2 comments.
Show a summary per file
File | Description |
---|---|
lib/schemaType.js, lib/schema/* | Add autoEncryptionType methods to various schema types returning proper BSON type strings |
lib/schema.js | Incorporates encryption options, encryptedFields tracking, and new API for encryptionType |
lib/model.js | Adds Model.clientEncryption helper for obtaining a ClientEncryption instance |
lib/helpers/model/discriminator.js | Validates that encrypted fields are not duplicated across discriminator schemas |
lib/drivers/node-mongodb-native/* | Exports ClientEncryption and builds combined encryption schemas within the connection logic |
docs/field-level-encryption.md | Documents automatic FLE support and usage of CSFLE/QE encryption |
.github/workflows/encryption-tests.yml | Updates workflow triggers to include a new branch |
.eslintrc.js | Adjusts excluded paths for linting |
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.
This is one big PR, it mostly looks good to me, but there are some minor style nitpicks and general suggestions / questions.
Additional question not related to a specific file & line:
- I did not see SchemaType
UUID
get aautoEncryptionType
or any tests as the field type, isUUID
even possible to be used as such? - Should there be tests for encrypting a whole subdocument which contains types that dont have
autoEncryptionType
(egMixed
)? - Should indexes on encrypted fields be handled or tested (though it does not seem to logical to me)?
Also finally, shouldnt this PR target branch 8.15
?
@@ -80,6 +125,8 @@ module.exports = function discriminator(model, name, schema, tiedValue, applyPlu | |||
value = tiedValue; | |||
} | |||
|
|||
validateDiscriminatorSchemasForEncryption(model.schema, schema); |
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 am not quite sure where this is in the discriminator chain, but if for example a child schema is passed-in that is already a clone of the parent, like typegoose currently does, wouldnt this also throw "duplicate encryption field" errors? Is this a case that should be supported?
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.
Could you elaborate on what exactly you mean by "a child discriminator that is a clone of the parent"? Do you mean that the child already has all fields from the parent added to it as well? Or is there something else you're referring to here?
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.
@baileympearson some devs prefer to pass a schema with the parent parents included in Model.discriminator()
. For example:
const baseSchema = new Schema({ name: String });
const BaseModel = mongoose.model('Base', baseSchema);
// `discSchema` contains a copy of `baseSchema` plus an extra field
const discSchema = baseSchema.clone().add({ age: Number });
// The following should work even though `discSchema` contains some duplicate fields
const DiscModel = BaseModel.discriminator('Disc', discSchema);
Can you add a test case for that please?
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 added tests in #15407. Just to be clear though - the implementation does not support this pattern and the tests I added demonstrate that this code throws
Re: "Should there be tests for encrypting a whole subdocument which contains types that dont have autoEncryptionType(eg Mixed)?", I agree with @baileympearson 's comments on #15404. You can't encrypt a whole subdocument, MongoDB only supports automatic encryption on certain primitive types - string, int32, int64, double, decimal128, date, binary, boolean, objectId. |
chore: address comments on CSFLE feature branch review
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.
PR now looks good to me.
2.Should there be tests for encrypting a whole subdocument which contains types that dont have autoEncryptionType(eg Mixed)?
Can you elaborate on this? I don't understand this scenario. Mixed isn't supported as an encryption type.
Re: "Should there be tests for encrypting a whole subdocument which contains types that dont have autoEncryptionType(eg Mixed)?", I agree with @baileympearson 's comments on #15404. You can't encrypt a whole subdocument, MongoDB only supports automatic encryption on certain primitive types - string, int32, int64, double, decimal128, date, binary, boolean, objectId.
Thanks for clarifying, from the tests it looked like subdocuments might be supported (bson type object
), so i just wanted to know if the case of subdocument(object) which contains a property without a autoEncryptionType
should throw or still encrypt somehow (via toJSON / fromJSON?).
But skimming the changes again and @vkarpov15's comment, i notice that subdocuments are not handled at all, so this point is kinda moot for now.
test/encryption/encryption.test.js
Outdated
* @param { string } path | ||
* | ||
* @returns { boolean } |
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.
* @param { string } path | |
* | |
* @returns { boolean } | |
* @param {string} path | |
* | |
* @returns {boolean} |
test/encryption/encryption.test.js
Outdated
const doc = await model.findOne({ _id }); | ||
assert.deepEqual(doc.a, 2); | ||
afterEach(async function() { | ||
await connection.close(); |
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.
await connection.close(); | |
await connection?.close(); |
Just to be sure that this will never throw if setup somehow fails.
chore: comments on CSFLE feature branch PR#2
Summary
@baileympearson 's support for CSFLE in Mongoose 👍
Examples