-
Notifications
You must be signed in to change notification settings - Fork 0
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
feat(NODE-6506): Add support for encrypted schemas #5
base: 8.10
Are you sure you want to change the base?
Conversation
d2412a6
to
6c469ec
Compare
027d005
to
f00f123
Compare
'data', | ||
'.config' |
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 .config because I noticed lint failed after generating the docs locally, and data
because otherwise the data for encrypted tests is linted.
lib/encryption_utils.js
Outdated
* @param {string} path | ||
* @returns | ||
*/ | ||
function inferBSONType(schema, path) { |
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.
Can we instead add these as a getBSONType()
method on SchemaType class? That would make this more extensible for custom schematypes.
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 can instead make it a getter on SchemaType
- but this isn't intended to be extensible for custom schema types (see the When a schema is instantiated with a custom schema type plugin
test).
FLE has pretty strict requirements about which bson types are supported and I intentionally only supported the exact schema type that correspond to FLE's supported schema types. Allowing users to provide custom schema types that get auto encrypted seems like it could confusion for users and opaque errors. A hypothetical scenario: a custom schema type sometimes casts to a support BSON type, sometimes it doesn't, causing FLE to throw only sometimes for users.
I'm open to reconsidering though - we could instead support any SchemaType that casts to exactly one supported BSON type. I'm not sure we could enforce this programmatically but documentation would probably suffice. Would you prefer that approach?
lib/encryption_utils.js
Outdated
@@ -0,0 +1,72 @@ | |||
'use strict'; |
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.
For consistency, please name this file lib/encryptionUtils.js
, Mongoose camelCases file names
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.
Does this apply to test files too?
|
||
if (val.instanceOfSchema && val.encryptionType() != null) { | ||
// schema.add({ field: <instance of encrypted schema> }) | ||
if (this.encryptionType() != val.encryptionType()) { |
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.
Mongoose document arrays get implicitly created schemas, so docArr: [{ name: String }]
implicitly creates a schema with { name: String }
. We pass certain schema options to implicitly created schemas, like typeKey
. It might be worthwhile to make sure encryption type gets passed down as well.
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.
Arrays are encrypted as an array, not by individual element. So the logic here only creates a schemaType of type "array" - the contents of the array don't matter. So encryptionType for the implicitly generated schemas shouldn't matter.
That being said, I'm happy to ensure that the encryption type gets propagated to sub-schemas if you'd like
lib/schema.js
Outdated
@@ -128,6 +130,8 @@ function Schema(obj, options) { | |||
// For internal debugging. Do not use this to try to save a schema in MDB. | |||
this.$id = ++id; | |||
this.mapPaths = []; | |||
this.encryptedFields = {}; | |||
this._encryptionType = options?.encryptionType; |
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.
Why is _encryptionType
a separate property, instead of making encryptionType()
get/set options.encryptionType
? The latter would be more consistent with how Mongoose handles other options.
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.
Only because I didn't consider mutation the options object 🙂 . Fixed!
docs/field-level-encryption.md
Outdated
|
||
### Encryption types | ||
|
||
MongoDB has to different automatic encryption implementations: client side field level encryption (CSFLE) and queryable encryption (QE). |
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.
MongoDB has to different automatic encryption implementations: client side field level encryption (CSFLE) and queryable encryption (QE). | |
MongoDB has two different automatic encryption implementations: client side field level encryption (CSFLE) and queryable encryption (QE). |
|
||
const schema2 = originalSchema.omit(['name', 'age']); | ||
|
||
assert.equal(schema2.encryptionType(), 'csfle'); |
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.
If we only pick
unencrypted fields, the resulting schema's options.encryptionType
is null
.
With the current implementation, if we omit
all encrypted fields, the resulting schema's options.encryptionType
is not null
. What's our reasoning for this?
4173fed
to
86ae2c0
Compare
86ae2c0
to
d02dfc4
Compare
Co-authored-by: Aditi Khare <106987683+aditi-khare-mongoDB@users.noreply.github.com>
Co-authored-by: Aditi Khare <106987683+aditi-khare-mongoDB@users.noreply.github.com>
Summary
This PR adds support for declaring encrypted schemas, as the first step in adding first class support for CSFLE to mongoose. Functionally:
encryptionType
, has been added. This is required for schemas that are declaring encrypted fields, and it determines whether the schema will be configured for 'csfle' or 'qe'.encrypt
option. This option contains metadata for libmongocrypt to encrypt the field (which will be automatically included in a schemaMap or encryptedFieldsMap). The contents of this document are exactly the same as the fields used to configure a field for csfle or qe, except thatbsonType
is not required (inferred from the schema type).This PR also updates all the schema modifiers / cloning methods to account for updating encrypted fields as well.
Examples
Declare an encrypted schema:
Modify / clone encrypted schemas