From d6899a3d8b436f1a85a87d19950806b72444bf6d Mon Sep 17 00:00:00 2001 From: Bailey Pearson Date: Thu, 15 Jun 2023 14:35:40 -0600 Subject: [PATCH 1/4] fix sasl prep issue --- src/cmap/auth/scram.ts | 12 +- test/integration/auth/scram_sha_256.test.js | 122 -------------------- test/integration/auth/scram_sha_256.test.ts | 41 +++++++ test/mongodb.ts | 1 + 4 files changed, 51 insertions(+), 125 deletions(-) delete mode 100644 test/integration/auth/scram_sha_256.test.js create mode 100644 test/integration/auth/scram_sha_256.test.ts diff --git a/src/cmap/auth/scram.ts b/src/cmap/auth/scram.ts index 96369e80dda..67b56300169 100644 --- a/src/cmap/auth/scram.ts +++ b/src/cmap/auth/scram.ts @@ -2,7 +2,7 @@ import * as crypto from 'crypto'; import { promisify } from 'util'; import { Binary, type Document } from '../../bson'; -import { saslprep } from '../../deps'; +import * as deps from '../../deps'; import { MongoInvalidArgumentError, MongoMissingCredentialsError, @@ -34,7 +34,10 @@ class ScramSHA extends AuthProvider { if (!credentials) { throw new MongoMissingCredentialsError('AuthContext must provide credentials.'); } - if (cryptoMethod === 'sha256' && saslprep == null) { + if ( + cryptoMethod === 'sha256' && + (!('kModuleError' in deps.saslprep) || typeof deps.saslprep !== 'function') + ) { emitWarning('Warning: no saslprep library specified. Passwords will not be sanitized'); } @@ -140,7 +143,10 @@ async function continueScramConversation( let processedPassword; if (cryptoMethod === 'sha256') { - processedPassword = 'kModuleError' in saslprep ? password : saslprep(password); + processedPassword = + 'kModuleError' in deps.saslprep || typeof deps.saslprep !== 'function' + ? password + : deps.saslprep(password); } else { processedPassword = passwordDigest(username, password); } diff --git a/test/integration/auth/scram_sha_256.test.js b/test/integration/auth/scram_sha_256.test.js deleted file mode 100644 index 889064d2880..00000000000 --- a/test/integration/auth/scram_sha_256.test.js +++ /dev/null @@ -1,122 +0,0 @@ -'use strict'; - -const sinon = require('sinon'); -const { expect } = require('chai'); -const { Connection } = require('../../mongodb'); -const { ScramSHA256 } = require('../../mongodb'); -const { setupDatabase } = require('../shared'); -const { LEGACY_HELLO_COMMAND } = require('../../mongodb'); - -// TODO(NODE-4338): withClient usage prevented these tests from running -// the import has been removed since the function is being deleted, this is here to keep modifications minimal -// so that the implementer of the fix for these tests can try to reference the original intent -const withClient = () => null; - -describe('SCRAM_SHA_256', function () { - beforeEach(function () { - this.currentTest.skipReason = 'TODO(NODE-4338): correct withClient usage'; - this.currentTest.skip(); - }); - - const test = {}; - - // Note: this setup was adapted from the prose test setup - const userMap = { - both: { - description: 'user with both credentials', - username: 'both', - password: 'both', - mechanisms: ['SCRAM-SHA-1', 'SCRAM-SHA-256'] - } - }; - - const users = Object.keys(userMap).map(name => userMap[name]); - - afterEach(() => test.sandbox.restore()); - - before(function () { - test.sandbox = sinon.createSandbox(); - return setupDatabase(this.configuration); - }); - - before(function () { - return withClient(this.configuration.newClient(), client => { - test.oldDbName = this.configuration.db; - this.configuration.db = 'admin'; - const db = client.db(this.configuration.db); - - const createUserCommands = users.map(user => ({ - createUser: user.username, - pwd: user.password, - roles: ['root'], - mechanisms: user.mechanisms - })); - - return Promise.all(createUserCommands.map(cmd => db.command(cmd))); - }); - }); - - after(function () { - return withClient(this.configuration.newClient(), client => { - const db = client.db(this.configuration.db); - this.configuration.db = test.oldDbName; - - return Promise.all(users.map(user => db.removeUser(user.username))); - }); - }); - - it('should shorten SCRAM conversations if the server supports it', { - metadata: { requires: { mongodb: '>=4.4', topology: ['single'] } }, - test: function () { - const options = { - auth: { - username: userMap.both.username, - password: userMap.both.password - }, - authSource: this.configuration.db - }; - - let runCommandSpy; - test.sandbox.stub(ScramSHA256.prototype, 'auth').callsFake(function (authContext, callback) { - const connection = authContext.connection; - const auth = ScramSHA256.prototype.auth.wrappedMethod; - runCommandSpy = test.sandbox.spy(connection, 'command'); - function _callback(err, res) { - runCommandSpy.restore(); - callback(err, res); - } - - auth.apply(this, [authContext, _callback]); - }); - - return withClient(this.configuration.newClient({}, options), () => { - expect(runCommandSpy.callCount).to.equal(1); - }); - } - }); - - it('should send speculativeAuthenticate on initial handshake on MongoDB 4.4+', { - metadata: { requires: { mongodb: '>=4.4', topology: ['single'] } }, - test: function () { - const options = { - auth: { - username: userMap.both.username, - password: userMap.both.password - }, - authSource: this.configuration.db - }; - - const commandSpy = test.sandbox.spy(Connection.prototype, 'command'); - return withClient(this.configuration.newClient({}, options), () => { - const calls = commandSpy - .getCalls() - .filter(c => c.thisValue.id !== '') // ignore all monitor connections - .filter(c => c.args[1][LEGACY_HELLO_COMMAND]); // only consider handshakes - - expect(calls).to.have.length(1); - const handshakeDoc = calls[0].args[1]; - expect(handshakeDoc).to.have.property('speculativeAuthenticate'); - }); - } - }); -}); diff --git a/test/integration/auth/scram_sha_256.test.ts b/test/integration/auth/scram_sha_256.test.ts new file mode 100644 index 00000000000..a8b685a7939 --- /dev/null +++ b/test/integration/auth/scram_sha_256.test.ts @@ -0,0 +1,41 @@ +import { expect } from 'chai'; +import * as sinon from 'sinon'; + +import { deps, type MongoClient } from '../../mongodb'; + +describe('SCRAM_SHA_256', function () { + context('when saslprep is not a function', () => { + let client: MongoClient; + beforeEach(function () { + if (!this.configuration.parameters.authenticationMechanisms.includes('SCRAM-SHA-256')) { + this.currentTest!.skipReason = 'Test requires that SCRAM-SHA-256 be enabled on the server.'; + this.currentTest!.skip(); + } + }); + + beforeEach('setup mocks', function () { + sinon.stub(deps, 'saslprep').value({}); + client = this.configuration.newClient({ authMechanism: 'SCRAM-SHA-256' }); + }); + + afterEach(() => { + sinon.restore(); + return client.close(); + }); + + it('does not throw an error', { requires: { auth: 'enabled' } }, async function () { + await client.connect(); + }); + + it('emits a warning', { requires: { auth: 'enabled' } }, async function () { + const warnings: Array = []; + process.once('warning', w => warnings.push(w)); + await client.connect(); + expect(warnings).to.have.lengthOf(1); + expect(warnings[0]).to.have.property( + 'message', + 'Warning: no saslprep library specified. Passwords will not be sanitized' + ); + }); + }); +}); diff --git a/test/mongodb.ts b/test/mongodb.ts index 53ea38256c0..0ce5dda5a2a 100644 --- a/test/mongodb.ts +++ b/test/mongodb.ts @@ -143,6 +143,7 @@ export * from '../src/cursor/list_indexes_cursor'; export * from '../src/cursor/run_command_cursor'; export * from '../src/db'; export * from '../src/deps'; +export * as deps from '../src/deps'; export * from '../src/encrypter'; export * from '../src/error'; export * from '../src/explain'; From 0771296a4bc36f192638ba8407c5c51f7282c0e3 Mon Sep 17 00:00:00 2001 From: Bailey Pearson Date: Fri, 16 Jun 2023 08:55:11 -0600 Subject: [PATCH 2/4] address comments --- src/cmap/auth/scram.ts | 8 +++----- test/integration/auth/scram_sha_256.test.ts | 6 +++++- 2 files changed, 8 insertions(+), 6 deletions(-) diff --git a/src/cmap/auth/scram.ts b/src/cmap/auth/scram.ts index 67b56300169..87bfbb0e863 100644 --- a/src/cmap/auth/scram.ts +++ b/src/cmap/auth/scram.ts @@ -2,7 +2,7 @@ import * as crypto from 'crypto'; import { promisify } from 'util'; import { Binary, type Document } from '../../bson'; -import * as deps from '../../deps'; +import { saslprep } from '../../deps'; import { MongoInvalidArgumentError, MongoMissingCredentialsError, @@ -36,7 +36,7 @@ class ScramSHA extends AuthProvider { } if ( cryptoMethod === 'sha256' && - (!('kModuleError' in deps.saslprep) || typeof deps.saslprep !== 'function') + ('kModuleError' in saslprep || typeof saslprep !== 'function') ) { emitWarning('Warning: no saslprep library specified. Passwords will not be sanitized'); } @@ -144,9 +144,7 @@ async function continueScramConversation( let processedPassword; if (cryptoMethod === 'sha256') { processedPassword = - 'kModuleError' in deps.saslprep || typeof deps.saslprep !== 'function' - ? password - : deps.saslprep(password); + 'kModuleError' in saslprep || typeof saslprep !== 'function' ? password : saslprep(password); } else { processedPassword = passwordDigest(username, password); } diff --git a/test/integration/auth/scram_sha_256.test.ts b/test/integration/auth/scram_sha_256.test.ts index a8b685a7939..57e49b80f13 100644 --- a/test/integration/auth/scram_sha_256.test.ts +++ b/test/integration/auth/scram_sha_256.test.ts @@ -1,14 +1,18 @@ import { expect } from 'chai'; import * as sinon from 'sinon'; -import { deps, type MongoClient } from '../../mongodb'; +// eslint-disable-next-line @typescript-eslint/no-restricted-imports +import * as deps from '../../../src/deps'; +import { type MongoClient } from '../../mongodb'; describe('SCRAM_SHA_256', function () { context('when saslprep is not a function', () => { let client: MongoClient; beforeEach(function () { if (!this.configuration.parameters.authenticationMechanisms.includes('SCRAM-SHA-256')) { + // eslint-disable-next-line @typescript-eslint/no-non-null-assertion this.currentTest!.skipReason = 'Test requires that SCRAM-SHA-256 be enabled on the server.'; + // eslint-disable-next-line @typescript-eslint/no-non-null-assertion this.currentTest!.skip(); } }); From 6b85764f9e30a003eb52fb13738f318eda908f2c Mon Sep 17 00:00:00 2001 From: Bailey Pearson Date: Fri, 16 Jun 2023 09:31:50 -0600 Subject: [PATCH 3/4] comments --- test/integration/auth/scram_sha_256.test.ts | 37 ++++++++++++++++----- test/mongodb.ts | 1 - 2 files changed, 28 insertions(+), 10 deletions(-) diff --git a/test/integration/auth/scram_sha_256.test.ts b/test/integration/auth/scram_sha_256.test.ts index 57e49b80f13..5c950f51612 100644 --- a/test/integration/auth/scram_sha_256.test.ts +++ b/test/integration/auth/scram_sha_256.test.ts @@ -6,18 +6,19 @@ import * as deps from '../../../src/deps'; import { type MongoClient } from '../../mongodb'; describe('SCRAM_SHA_256', function () { + beforeEach(function () { + if (!this.configuration.parameters.authenticationMechanisms.includes('SCRAM-SHA-256')) { + // eslint-disable-next-line @typescript-eslint/no-non-null-assertion + this.currentTest!.skipReason = 'Test requires that SCRAM-SHA-256 be enabled on the server.'; + // eslint-disable-next-line @typescript-eslint/no-non-null-assertion + this.currentTest!.skip(); + } + }); + context('when saslprep is not a function', () => { let client: MongoClient; - beforeEach(function () { - if (!this.configuration.parameters.authenticationMechanisms.includes('SCRAM-SHA-256')) { - // eslint-disable-next-line @typescript-eslint/no-non-null-assertion - this.currentTest!.skipReason = 'Test requires that SCRAM-SHA-256 be enabled on the server.'; - // eslint-disable-next-line @typescript-eslint/no-non-null-assertion - this.currentTest!.skip(); - } - }); - beforeEach('setup mocks', function () { + beforeEach(function () { sinon.stub(deps, 'saslprep').value({}); client = this.configuration.newClient({ authMechanism: 'SCRAM-SHA-256' }); }); @@ -42,4 +43,22 @@ describe('SCRAM_SHA_256', function () { ); }); }); + + context('when saslprep is a function', () => { + let client: MongoClient; + + beforeEach(function () { + client = this.configuration.newClient({ authMechanism: 'SCRAM-SHA-256' }); + }); + + afterEach(() => client.close()); + + it('calls saslprep', { requires: { auth: 'enabled' } }, async function () { + const spy = sinon.spy(deps, 'saslprep'); + + await client.connect(); + + expect(spy.called).to.be.true; + }); + }); }); diff --git a/test/mongodb.ts b/test/mongodb.ts index 0ce5dda5a2a..53ea38256c0 100644 --- a/test/mongodb.ts +++ b/test/mongodb.ts @@ -143,7 +143,6 @@ export * from '../src/cursor/list_indexes_cursor'; export * from '../src/cursor/run_command_cursor'; export * from '../src/db'; export * from '../src/deps'; -export * as deps from '../src/deps'; export * from '../src/encrypter'; export * from '../src/error'; export * from '../src/explain'; From 385ef7fc5b14ee8210685edc0d56734a3c43ce24 Mon Sep 17 00:00:00 2001 From: Bailey Pearson Date: Fri, 16 Jun 2023 13:49:13 -0600 Subject: [PATCH 4/4] Update test/integration/auth/scram_sha_256.test.ts Co-authored-by: Neal Beeken --- test/integration/auth/scram_sha_256.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/integration/auth/scram_sha_256.test.ts b/test/integration/auth/scram_sha_256.test.ts index 5c950f51612..2940ada09bd 100644 --- a/test/integration/auth/scram_sha_256.test.ts +++ b/test/integration/auth/scram_sha_256.test.ts @@ -58,7 +58,7 @@ describe('SCRAM_SHA_256', function () { await client.connect(); - expect(spy.called).to.be.true; + expect(spy).to.have.been.called; }); }); });