Skip to content
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

Open
wants to merge 3 commits into
base: 8.10
Choose a base branch
from

Conversation

baileympearson
Copy link

@baileympearson baileympearson commented Dec 26, 2024

Summary

This PR adds support for declaring encrypted schemas, as the first step in adding first class support for CSFLE to mongoose. Functionally:

  1. A new schema option, 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'.
  2. Keys in schemas can now be configured with an additional 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 that bsonType 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:

const encryptedSchema = new Schema({
  name: { type: String, encrypt: { keyId: '...' } },
  friend: { 
      // nested objects allowed
      name: { type: String, encrypt: { keyId: '...' } },
  },
  // arrays allowed - but becase of how CSFLE/QE encrypted arrays, the arrays are encrypted as a whole
  books: { type: [String], encrypt: { keyId: '...' } }
}, { encryptionType: 'qe' });

Modify / clone encrypted schemas

encryptedSchema.clone();

encryptedSchema.add({ name: String }) // name is no longer encrypted
encryptedSchema.add({ newKey: { type: String, encrypt: { keyId: '...' } }); // new encrypted key added

encryptedSchema.remove('name'); // name no longer in schema at all

encryptedSchema.pick('friends'); // returns a new schema with only one field, `friends`, that is encrypted.

encryptedSchema.omit('friends'); // returns a new schema with all fields except `frields`

@baileympearson baileympearson marked this pull request as draft December 26, 2024 18:59
@baileympearson baileympearson force-pushed the NODE-6506-add-support-for-encrypted-schemas branch from d2412a6 to 6c469ec Compare January 11, 2025 22:07
@baileympearson baileympearson changed the title feat(NODE-xxxx): Add support for encrypted schemas feat(NODE-6506): Add support for encrypted schemas Jan 13, 2025
@baileympearson baileympearson force-pushed the NODE-6506-add-support-for-encrypted-schemas branch 3 times, most recently from 027d005 to f00f123 Compare January 13, 2025 16:20
@baileympearson baileympearson marked this pull request as ready for review January 13, 2025 16:41
Comment on lines +18 to +19
'data',
'.config'
Copy link
Author

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.

@nbbeeken nbbeeken self-assigned this Jan 15, 2025
lib/encryption_utils.js Outdated Show resolved Hide resolved
test/encrypted_schema.test.js Outdated Show resolved Hide resolved
docs/field-level-encryption.md Outdated Show resolved Hide resolved
lib/schema.js Outdated Show resolved Hide resolved
* @param {string} path
* @returns
*/
function inferBSONType(schema, path) {
Copy link
Member

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.

Copy link
Author

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?

@@ -0,0 +1,72 @@
'use strict';
Copy link
Member

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

Copy link
Author

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()) {
Copy link
Member

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.

Copy link
Author

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;
Copy link
Member

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.

Copy link
Author

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!


### Encryption types

MongoDB has to different automatic encryption implementations: client side field level encryption (CSFLE) and queryable encryption (QE).

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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).

lib/encryptionUtils.js Outdated Show resolved Hide resolved
test/encrypted_schema.test.js Outdated Show resolved Hide resolved

const schema2 = originalSchema.omit(['name', 'age']);

assert.equal(schema2.encryptionType(), 'csfle');

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?

@baileympearson baileympearson force-pushed the NODE-6506-add-support-for-encrypted-schemas branch from 4173fed to 86ae2c0 Compare January 23, 2025 15:30
@baileympearson baileympearson force-pushed the NODE-6506-add-support-for-encrypted-schemas branch from 86ae2c0 to d02dfc4 Compare January 23, 2025 15:41
@baileympearson baileympearson changed the base branch from master to 8.10 January 23, 2025 15:42
baileympearson and others added 2 commits January 23, 2025 08:43
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants