Skip to content

fix(NODE-5053): enforce empty map for kmsProvider auto credentials #565

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

Merged
merged 6 commits into from
Feb 10, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
64 changes: 45 additions & 19 deletions bindings/node/lib/credentialsProvider.js
Original file line number Diff line number Diff line change
@@ -1,18 +1,27 @@
'use strict';

let awsCredentialProviders = null;

/**
* Load cloud provider credentials for the user provided KMS providers.
* Credentials will only attempt to get loaded if they do not exist
* and no existing credentials will get overwritten.
* @ignore
* Auto credential fetching should only occur when the provider is defined on the kmsProviders map
* and the settings are an empty object.
*
* @param {Object} kmsProviders - The user provided KMS providers.
* @returns {Promise} The new kms providers.
* This is distinct from a nullish provider key.
*
* @ignore
* @param {string} provider
* @param {object} kmsProviders
*/
async function loadCredentials(kmsProviders) {
function isEmptyCredentials(provider, kmsProviders) {
return (
provider in kmsProviders &&
kmsProviders[provider] != null &&
typeof kmsProviders[provider] === 'object' &&
Object.keys(kmsProviders[provider]).length === 0
);
}

let awsCredentialProviders = null;
Copy link
Contributor

Choose a reason for hiding this comment

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

Btw, it is so nice that this package does dependency injection for the driver package – It might even be worth adapting that to this dependency as well … :)

Copy link
Contributor

Choose a reason for hiding this comment

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

How come you think this is a beneficial pattern? The DI for the driver is something I've wanted to remove for a while

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, from a very practical point, it’s just a necessity in cases where the driver is bundled into something but this package isn’t (which would be common in bundling scenarios given that this package is a native addon package).

But also just philosophically (and I’m pretty sure you already know this), I think DI is a great design pattern that solves more issues than it creates 🙂

Copy link
Contributor

Choose a reason for hiding this comment

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

Just to clarify, this thought is not directly related to the changes in this PR and should absolutely not block it :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We should consider this though, using it for testing too could be 🔥

For GCP we don't plan to use an SDK (and likely not for azure either) so this would only be for AWS but still, valid use case even for one dep.

/** @ignore */
async function loadAWSCredentials(kmsProviders) {
if (awsCredentialProviders == null) {
try {
// Ensure you always wrap an optional require in the try block NODE-3199
Expand All @@ -22,18 +31,35 @@ async function loadCredentials(kmsProviders) {
}

if (awsCredentialProviders != null) {
const aws = kmsProviders.aws;
if (!aws || Object.keys(aws).length === 0) {
const { fromNodeProviderChain } = awsCredentialProviders;
const provider = fromNodeProviderChain();
// The state machine is the only place calling this so it will
// catch if there is a rejection here.
const awsCreds = await provider();
return { ...kmsProviders, aws: awsCreds };
}
const { fromNodeProviderChain } = awsCredentialProviders;
const provider = fromNodeProviderChain();
// The state machine is the only place calling this so it will
// catch if there is a rejection here.
const aws = await provider();
return { ...kmsProviders, aws };
}

return kmsProviders;
}

module.exports = { loadCredentials };
/**
* Load cloud provider credentials for the user provided KMS providers.
* Credentials will only attempt to get loaded if they do not exist
* and no existing credentials will get overwritten.
*
* @param {object} kmsProviders - The user provided KMS providers.
* @returns {Promise} The new kms providers.
*
* @ignore
*/
async function loadCredentials(kmsProviders) {
let finalKMSProviders = kmsProviders;

if (isEmptyCredentials('aws', kmsProviders)) {
finalKMSProviders = await loadAWSCredentials(kmsProviders);
}

return finalKMSProviders;
}

module.exports = { loadCredentials, isEmptyCredentials };
210 changes: 102 additions & 108 deletions bindings/node/test/credentialsProvider.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,62 +2,56 @@

const { expect } = require('chai');
const requirements = require('./requirements.helper');
const { loadCredentials } = require('../lib/credentialsProvider');
const { loadCredentials, isEmptyCredentials } = require('../lib/credentialsProvider');

const originalAccessKeyId = process.env.AWS_ACCESS_KEY_ID;
const originalSecretAccessKey = process.env.AWS_SECRET_ACCESS_KEY;
const originalSessionToken = process.env.AWS_SESSION_TOKEN;

describe('#loadCredentials', function () {
const accessKey = 'example';
const secretKey = 'example';
const sessionToken = 'example';

after(function () {
// After the entire suite runs, set the env back for the rest of the test run.
process.env.AWS_ACCESS_KEY_ID = originalAccessKeyId;
process.env.AWS_SECRET_ACCESS_KEY = originalSecretAccessKey;
process.env.AWS_SESSION_TOKEN = originalSessionToken;
});
context('isEmptyCredentials()', () => {
it('returns true for an empty object', () => {
expect(isEmptyCredentials('rainyCloud', { rainyCloud: {} })).to.be.true;
});

context('when the credential provider finds credentials', function () {
before(function () {
process.env.AWS_ACCESS_KEY_ID = accessKey;
process.env.AWS_SECRET_ACCESS_KEY = secretKey;
process.env.AWS_SESSION_TOKEN = sessionToken;
it('returns false for an object with keys', () => {
expect(isEmptyCredentials('rainyCloud', { rainyCloud: { password: 'secret' } })).to.be.false;
});

context('when the credentials are empty', function () {
const kmsProviders = {};
it('returns false for an nullish credentials', () => {
expect(isEmptyCredentials('rainyCloud', { rainyCloud: null })).to.be.false;
expect(isEmptyCredentials('rainyCloud', { rainyCloud: undefined })).to.be.false;
expect(isEmptyCredentials('rainyCloud', {})).to.be.false;
});

before(function () {
if (!requirements.credentialProvidersInstalled.aws) {
this.currentTest.skipReason = 'Cannot refresh credentials without sdk provider';
this.currentTest.skip();
return;
}
});
it('returns false for non object credentials', () => {
expect(isEmptyCredentials('rainyCloud', { rainyCloud: 0 })).to.be.false;
expect(isEmptyCredentials('rainyCloud', { rainyCloud: false })).to.be.false;
expect(isEmptyCredentials('rainyCloud', { rainyCloud: Symbol('secret') })).to.be.false;
});
});

it('refreshes the aws credentials', async function () {
const providers = await loadCredentials(kmsProviders);
expect(providers).to.deep.equal({
aws: {
accessKeyId: accessKey,
secretAccessKey: secretKey,
sessionToken: sessionToken
}
});
});
context('when using aws', () => {
const accessKey = 'example';
const secretKey = 'example';
const sessionToken = 'example';

after(function () {
// After the entire suite runs, set the env back for the rest of the test run.
process.env.AWS_ACCESS_KEY_ID = originalAccessKeyId;
process.env.AWS_SECRET_ACCESS_KEY = originalSecretAccessKey;
process.env.AWS_SESSION_TOKEN = originalSessionToken;
});

context('when the credentials are not empty', function () {
context('when aws is empty', function () {
const kmsProviders = {
local: {
key: Buffer.alloc(96)
},
aws: {}
};
context('when the credential provider finds credentials', function () {
before(function () {
process.env.AWS_ACCESS_KEY_ID = accessKey;
process.env.AWS_SECRET_ACCESS_KEY = secretKey;
process.env.AWS_SESSION_TOKEN = sessionToken;
});

context('when the credentials are empty', function () {
const kmsProviders = { aws: {} };

before(function () {
if (!requirements.credentialProvidersInstalled.aws) {
Expand All @@ -67,12 +61,9 @@ describe('#loadCredentials', function () {
}
});

it('refreshes only the aws credentials', async function () {
it('refreshes the aws credentials', async function () {
const providers = await loadCredentials(kmsProviders);
expect(providers).to.deep.equal({
local: {
key: Buffer.alloc(96)
},
aws: {
accessKeyId: accessKey,
secretAccessKey: secretKey,
Expand All @@ -82,81 +73,84 @@ describe('#loadCredentials', function () {
});
});

context('when aws is not empty', function () {
const kmsProviders = {
local: {
key: Buffer.alloc(96)
},
aws: {
accessKeyId: 'example'
}
};

before(function () {
if (!requirements.credentialProvidersInstalled.aws) {
this.currentTest.skipReason = 'Cannot refresh credentials without sdk provider';
this.currentTest.skip();
return;
}
});

it('does not refresh credentials', async function () {
const providers = await loadCredentials(kmsProviders);
expect(providers).to.deep.equal(kmsProviders);
});
});

context('when aws does not exist', function () {
const kmsProviders = {
local: {
key: Buffer.alloc(96)
}
};
context('when the credentials are not empty', function () {
context('when aws is empty', function () {
const kmsProviders = {
local: {
key: Buffer.alloc(96)
},
aws: {}
};

before(function () {
if (!requirements.credentialProvidersInstalled.aws) {
this.currentTest.skipReason = 'Cannot refresh credentials without sdk provider';
this.currentTest.skip();
return;
}
});

before(function () {
if (!requirements.credentialProvidersInstalled.aws) {
this.currentTest.skipReason = 'Cannot refresh credentials without sdk provider';
this.currentTest.skip();
return;
}
it('refreshes only the aws credentials', async function () {
const providers = await loadCredentials(kmsProviders);
expect(providers).to.deep.equal({
local: {
key: Buffer.alloc(96)
},
aws: {
accessKeyId: accessKey,
secretAccessKey: secretKey,
sessionToken: sessionToken
}
});
});
});

it('refreshes ony the aws credentials', async function () {
const providers = await loadCredentials(kmsProviders);
expect(providers).to.deep.equal({
context('when aws is not empty', function () {
const kmsProviders = {
local: {
key: Buffer.alloc(96)
},
aws: {
accessKeyId: accessKey,
secretAccessKey: secretKey,
sessionToken: sessionToken
accessKeyId: 'example'
}
};

before(function () {
if (!requirements.credentialProvidersInstalled.aws) {
this.currentTest.skipReason = 'Cannot refresh credentials without sdk provider';
this.currentTest.skip();
return;
}
});

it('does not refresh credentials', async function () {
const providers = await loadCredentials(kmsProviders);
expect(providers).to.deep.equal(kmsProviders);
});
});
});
});
});

context('when the sdk is not installed', function () {
const kmsProviders = {
local: {
key: Buffer.alloc(96)
},
aws: {}
};

before(function () {
if (requirements.credentialProvidersInstalled.aws) {
this.currentTest.skipReason = 'Credentials will be loaded when sdk present';
this.currentTest.skip();
return;
}
});
context('when the sdk is not installed', function () {
const kmsProviders = {
local: {
key: Buffer.alloc(96)
},
aws: {}
};

it('does not refresh credentials', async function () {
const providers = await loadCredentials(kmsProviders);
expect(providers).to.deep.equal(kmsProviders);
before(function () {
if (requirements.credentialProvidersInstalled.aws) {
this.currentTest.skipReason = 'Credentials will be loaded when sdk present';
this.currentTest.skip();
return;
}
});

it('does not refresh credentials', async function () {
const providers = await loadCredentials(kmsProviders);
expect(providers).to.deep.equal(kmsProviders);
});
});
});
});
29 changes: 29 additions & 0 deletions bindings/node/test/index.test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
'use strict';

const { expect } = require('chai');
const mongodbClientEncryption = require('../lib/index');

// Update this as you add exports, helps double check we don't accidentally remove something
// since not all tests import from the root public export
const EXPECTED_EXPORTS = [
'extension',
'MongoCryptError',
'MongoCryptCreateEncryptedCollectionError',
'MongoCryptCreateDataKeyError',
'AutoEncrypter',
'ClientEncryption'
];

describe('mongodb-client-encryption entrypoint', () => {
it('should export all and only the expected keys in expected_exports', () => {
expect(mongodbClientEncryption).to.have.all.keys(EXPECTED_EXPORTS);
});

it('extension returns an object equal in shape to the default except for extension', () => {
const extensionResult = mongodbClientEncryption.extension(require('mongodb'));
const expectedExports = EXPECTED_EXPORTS.filter(exp => exp !== 'extension');
const exportsDefault = Object.keys(mongodbClientEncryption).filter(exp => exp !== 'extension');
expect(extensionResult).to.have.all.keys(expectedExports);
expect(extensionResult).to.have.all.keys(exportsDefault);
});
});