Skip to content

Commit

Permalink
fix(NODE-3118): keyAltNames option not serialized (#176)
Browse files Browse the repository at this point in the history
  • Loading branch information
@nbbeeken
nbbeeken authored Jun 22, 2021
1 parent 0e7a0fd commit be492d4
Show file tree
Hide file tree
Showing 8 changed files with 150 additions and 116 deletions.
74 changes: 32 additions & 42 deletions bindings/node/lib/clientEncryption.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,43 +9,6 @@ module.exports = function(modules) {
const StateMachine = modules.stateMachine.StateMachine;
const cryptoCallbacks = require('./cryptoCallbacks');

function sanitizeDataKeyOptions(bson, options) {
options = Object.assign({}, options);

// To avoid using libbson inside the bindings, we pre-serialize
// any keyAltNames here.
if (options.keyAltNames) {
if (!Array.isArray(options.keyAltNames)) {
throw new TypeError(
`Option "keyAltNames" must be an array of string, but was of type ${typeof options.keyAltNames}.`
);
}
const serializedKeyAltNames = [];
for (let i = 0; i < options.keyAltNames.length; i += 1) {
const item = options.keyAltNames[i];
const itemType = typeof item;
if (itemType !== 'string') {
throw new TypeError(
`Option "keyAltNames" must be an array of string, but item at index ${i} was of type ${itemType} `
);
}

serializedKeyAltNames.push(bson.serialize({ keyAltName: item }));
}

options.keyAltNames = serializedKeyAltNames;
} else if (options.keyAltNames == null) {
// If keyAltNames is null or undefined, we can assume the intent of
// the user is to not pass in the value. B/c Nan::Has will still
// register a value of null or undefined as present as long
// as the key is present, we delete it off of the options
// object here.
delete options.keyAltNames;
}

return options;
}

/**
* @typedef {object} KMSProviders
* @description Configuration options that are used by specific KMS providers during key generation, encryption, and decryption.
Expand Down Expand Up @@ -202,12 +165,39 @@ module.exports = function(modules) {
* });
*/
createDataKey(provider, options, callback) {
if (typeof options === 'function') (callback = options), (options = {});
if (typeof options === 'function') {
callback = options;
options = {};
}
if (typeof options === 'undefined') {
options = {};
}

const bson = this._bson;
options = sanitizeDataKeyOptions(bson, options);
const dataKeyBson = bson.serialize(Object.assign({ provider }, options.masterKey));
const context = this._mongoCrypt.makeDataKeyContext(dataKeyBson);

const dataKey = Object.assign({ provider }, options.masterKey);

if (options.keyAltNames && !Array.isArray(options.keyAltNames)) {
throw new TypeError(
`Option "keyAltNames" must be an array of strings, but was of type ${typeof options.keyAltNames}.`
);
}

let keyAltNames = undefined;
if (options.keyAltNames && options.keyAltNames.length > 0) {
keyAltNames = options.keyAltNames.map((keyAltName, i) => {
if (typeof keyAltName !== 'string') {
throw new TypeError(
`Option "keyAltNames" must be an array of strings, but item at index ${i} was of type ${typeof keyAltName}`
);
}

return bson.serialize({ keyAltName });
});
}

const dataKeyBson = bson.serialize(dataKey);
const context = this._mongoCrypt.makeDataKeyContext(dataKeyBson, { keyAltNames });
const stateMachine = new StateMachine({ bson });

return promiseOrCallback(callback, cb => {
Expand All @@ -223,7 +213,7 @@ module.exports = function(modules) {
this._keyVaultClient
.db(dbName)
.collection(collectionName)
.insertOne(dataKey, { w: 'majority' }, (err, result) => {
.insertOne(dataKey, { writeConcern: { w: 'majority' } }, (err, result) => {
if (err) {
cb(err, null);
return;
Expand Down
10 changes: 7 additions & 3 deletions bindings/node/lib/stateMachine.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,11 @@

module.exports = function(modules) {
const tls = require('tls');
const MongoTimeoutError = modules.mongodb.MongoTimeoutError;

// Try first to import 4.x name, fallback to 3.x name
const MongoNetworkTimeoutError =
modules.mongodb.MongoNetworkTimeoutError || modules.mongodb.MongoTimeoutError;

const common = require('./common');
const debug = common.debug;
const databaseNamespace = common.databaseNamespace;
Expand Down Expand Up @@ -59,7 +63,7 @@ module.exports = function(modules) {
* @ignore
* @callback StateMachine~fetchKeysCallback
* @param {Error} [err] If present, indicates that fetching the keys failed with the given error
* @param {object[]} [result] If present, is all the keys from the keyVault colleciton that matched the given filter
* @param {object[]} [result] If present, is all the keys from the keyVault collection that matched the given filter
*/

/**
Expand Down Expand Up @@ -116,7 +120,7 @@ module.exports = function(modules) {
if (err) {
// If we are not bypassing spawning, then we should retry once on a MongoTimeoutError (server selection error)
if (
err instanceof MongoTimeoutError &&
err instanceof MongoNetworkTimeoutError &&
mongocryptdManager &&
!mongocryptdManager.bypassSpawn
) {
Expand Down
45 changes: 31 additions & 14 deletions bindings/node/package-lock.json
View file

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 3 additions & 2 deletions bindings/node/package.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -38,15 +38,16 @@
"prebuild-install": "6.1.2"
},
"devDependencies": {
"bson": "^4.2.3",
"bson": "^4.4.0",
"chai": "^4.3.4",
"chai-subset": "^1.6.0",
"clang-format": "^1.5.0",
"dmd-clear": "^0.1.2",
"eslint": "^4.19.1",
"eslint-plugin-prettier": "^2.7.0",
"jsdoc-to-markdown": "^5.0.3",
"mocha": "^4.1.0",
"mongodb": "^3.5.9",
"mongodb": "^3.6.9",
"node-gyp": "^5.1.1",
"prebuild": "^10.0.1",
"prettier": "^1.19.1",
Expand Down
30 changes: 30 additions & 0 deletions bindings/node/src/mongocrypt.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -674,6 +674,36 @@ NAN_METHOD(MongoCrypt::MakeDataKeyContext) {
return;
}

v8::Local<v8::Object> options = Nan::To<v8::Object>(info[1]).ToLocalChecked();
auto ALT_NAMES_KEY = Nan::New("keyAltNames").ToLocalChecked();
if (Nan::Has(options, ALT_NAMES_KEY).FromMaybe(false)) {
v8::Local<v8::Value> keyAltNames = Nan::Get(options, ALT_NAMES_KEY).ToLocalChecked();

if (keyAltNames->IsArray()) {
v8::Local<v8::Array> keyAltNamesArray = v8::Local<v8::Array>::Cast(keyAltNames);
uint32_t keyAltNamesLength = keyAltNamesArray->Length();
for (uint32_t i = 0; i < keyAltNamesLength; i += 1) {
if (Nan::Has(keyAltNamesArray, i).FromMaybe(false)) {
v8::Local<v8::Object> keyAltName =
Nan::To<v8::Object>(Nan::Get(keyAltNamesArray, i).ToLocalChecked())
.ToLocalChecked();
if (!node::Buffer::HasInstance(keyAltName)) {
// We should never get here
Nan::ThrowTypeError("Serialized keyAltName must be a Buffer");
return;
}

std::unique_ptr<mongocrypt_binary_t, MongoCryptBinaryDeleter> binary(
BufferToBinary(keyAltName));
if (!mongocrypt_ctx_setopt_key_alt_name(context.get(), binary.get())) {
Nan::ThrowTypeError(errorStringFromStatus(context.get()));
return;
}
}
}
}
}

if (!mongocrypt_ctx_datakey_init(context.get())) {
Nan::ThrowTypeError(errorStringFromStatus(context.get()));
return;
Expand Down
19 changes: 8 additions & 11 deletions bindings/node/test/autoEncrypter.test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,8 @@ const fs = require('fs');
const BSON = require('bson');
const EJSON = require('bson').EJSON;
const sinon = require('sinon');
const mongodb = Object.assign({}, require('mongodb'), {
// TODO: once this is actually defined, use the real one
MongoTimeoutError: class MongoTimeoutError {}
});
const MongoTimeoutError = mongodb.MongoTimeoutError;
const mongodb = require('mongodb');
const MongoNetworkTimeoutError = mongodb.MongoNetworkTimeoutError || mongodb.MongoTimeoutError;
const stateMachine = require('../lib/stateMachine')({ mongodb });
const StateMachine = stateMachine.StateMachine;
const MongocryptdManager = require('../lib/mongocryptdManager').MongocryptdManager;
Expand Down Expand Up @@ -277,12 +274,12 @@ describe('AutoEncrypter', function() {
});
});

it('should restore the mongocryptd and retry once if a MongoTimeoutError is experienced', function(done) {
it('should restore the mongocryptd and retry once if a MongoNetworkTimeoutError is experienced', function(done) {
let called = false;
StateMachine.prototype.markCommand.callsFake((client, ns, filter, callback) => {
if (!called) {
called = true;
callback(new MongoTimeoutError('msg'));
callback(new MongoNetworkTimeoutError('msg'));
return;
}

Expand Down Expand Up @@ -313,12 +310,12 @@ describe('AutoEncrypter', function() {
});
});

it('should propagate error if MongoTimeoutError is experienced twice in a row', function(done) {
it('should propagate error if MongoNetworkTimeoutError is experienced twice in a row', function(done) {
let counter = 2;
StateMachine.prototype.markCommand.callsFake((client, ns, filter, callback) => {
if (counter) {
counter -= 1;
callback(new MongoTimeoutError('msg'));
callback(new MongoNetworkTimeoutError('msg'));
return;
}

Expand All @@ -343,7 +340,7 @@ describe('AutoEncrypter', function() {

this.mc.encrypt('test.test', TEST_COMMAND, err => {
expect(localMcdm.spawn).to.have.been.calledOnce;
expect(err).to.be.an.instanceof(MongoTimeoutError);
expect(err).to.be.an.instanceof(MongoNetworkTimeoutError);
done();
});
});
Expand Down Expand Up @@ -423,7 +420,7 @@ describe('AutoEncrypter', function() {

it('should not spawn a mongocryptd or retry on a server selection error', function(done) {
let called = false;
const timeoutError = new MongoTimeoutError('msg');
const timeoutError = new MongoNetworkTimeoutError('msg');
StateMachine.prototype.markCommand.callsFake((client, ns, filter, callback) => {
if (!called) {
called = true;
Expand Down
Loading

0 comments on commit be492d4

Please sign in to comment.