-
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.
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.
Works for me
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?
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 think I would prefer the more extensible approach just in case developers have their own custom type wrappers around built-in types. For example, users may still use mongoose-int32 for int32 support, or may have their own custom Price
type that gets stored as an int32 under the hood. Using custom types like this isn't a pattern that the Mongoose docs recommend, but people do all sorts of surprising things.
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?
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
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 <[email protected]>
Co-authored-by: Aditi Khare <[email protected]>
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