diff --git a/.circleci/config.yml b/.circleci/config.yml index 1d53029e2..36274d764 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -14,5 +14,5 @@ jobs: - run: make check-file-headers - run: npm ci --legacy-peer-deps - run: node lib/bin/create-docker-databases.js - - run: make test-full + - run: make test diff --git a/.github/PULL_REQUEST_TEMPLATE.md b/.github/PULL_REQUEST_TEMPLATE.md index 231f088cd..b87ed0248 100644 --- a/.github/PULL_REQUEST_TEMPLATE.md +++ b/.github/PULL_REQUEST_TEMPLATE.md @@ -17,5 +17,5 @@ https://github.com/getodk/central-backend/blob/master/CONTRIBUTING.md #### Before submitting this PR, please make sure you have: -- [ ] run `make test-full` and confirmed all checks still pass OR confirm CircleCI build passes +- [ ] run `make test` and confirmed all checks still pass OR confirm CircleCI build passes - [ ] verified that any code from external sources are properly credited in comments or that everything is internally sourced \ No newline at end of file diff --git a/.github/workflows/standard-suite.yml b/.github/workflows/standard-suite.yml index 0af050ef1..7d1c77eca 100644 --- a/.github/workflows/standard-suite.yml +++ b/.github/workflows/standard-suite.yml @@ -30,4 +30,4 @@ jobs: cache: 'npm' - run: npm ci --legacy-peer-deps - run: node lib/bin/create-docker-databases.js - - run: make test-full + - run: make test diff --git a/Makefile b/Makefile index 0e19b3f80..af9969b40 100644 --- a/Makefile +++ b/Makefile @@ -60,11 +60,7 @@ debug: base .PHONY: test test: lint - BCRYPT=no npx mocha --recursive --exit - -.PHONY: test-full -test-full: lint - npx mocha --recursive --exit + BCRYPT=insecure npx mocha --recursive --exit .PHONY: test-fast test-fast: node_version @@ -72,11 +68,11 @@ test-fast: node_version .PHONY: test-integration test-integration: node_version - BCRYPT=no npx mocha --recursive test/integration --exit + BCRYPT=insecure npx mocha --recursive test/integration --exit .PHONY: test-unit test-unit: node_version - npx mocha --recursive test/unit --exit + BCRYPT=insecure npx mocha --recursive test/unit --exit .PHONY: test-coverage test-coverage: node_version diff --git a/lib/bin/run-server.js b/lib/bin/run-server.js index 7dec7eed5..261cc8b78 100644 --- a/lib/bin/run-server.js +++ b/lib/bin/run-server.js @@ -34,7 +34,6 @@ Error.stackTrackLimit = 20; // get an xlsform client and a password module. const xlsform = require('../external/xlsform').init(config.get('default.xlsform')); -const bcrypt = require('../util/crypto').password(require('bcrypt')); // get an Enketo client const enketo = require('../external/enketo').init(config.get('default.enketo')); @@ -45,7 +44,7 @@ const enketo = require('../external/enketo').init(config.get('default.enketo')); // initialize our container, then generate an http service out of it. const container = require('../model/container') - .withDefaults({ db, mail, env, Sentry, bcrypt, xlsform, enketo }); + .withDefaults({ db, mail, env, Sentry, xlsform, enketo }); const service = require('../http/service')(container); // insert the graceful exit middleware. diff --git a/lib/http/preprocessors.js b/lib/http/preprocessors.js index 0c00bbac2..4874c6e5a 100644 --- a/lib/http/preprocessors.js +++ b/lib/http/preprocessors.js @@ -7,6 +7,7 @@ // including this file, may be copied, modified, propagated, or distributed // except according to the terms contained in the LICENSE file. +const { verifyPassword } = require('../util/crypto'); const { isBlank, isPresent, noop, without } = require('../util/util'); const { isTrue } = require('../util/http'); const oidc = require('../util/oidc'); @@ -27,7 +28,7 @@ const emptyAuthInjector = ({ Auth }, context) => context.with({ auth: Auth.by(nu // in conjunction with this function! // // TODO?: repetitive, but deduping it makes it even harder to understand. -const authHandler = ({ Sessions, Users, Auth, bcrypt }, context) => { +const authHandler = ({ Sessions, Users, Auth }, context) => { const authBySessionToken = (token, onFailure = noop) => Sessions.getByBearerToken(token) .then((session) => { if (!session.isDefined()) return onFailure(); @@ -87,7 +88,7 @@ const authHandler = ({ Sessions, Users, Auth, bcrypt }, context) => { // TODO: email existence timing attack on whether bcrypt runs or not. return Users.getByEmail(email) .then(getOrReject(Problem.user.authenticationFailed())) - .then((user) => bcrypt.verify(password, user.password) + .then((user) => verifyPassword(password, user.password) .then((verified) => { if (verified === true) return context.with({ auth: Auth.by(user.actor) }); diff --git a/lib/model/query/users.js b/lib/model/query/users.js index 77a7bf7bf..e3c8fa43f 100644 --- a/lib/model/query/users.js +++ b/lib/model/query/users.js @@ -10,6 +10,7 @@ const { sql } = require('slonik'); const { map } = require('ramda'); const { Actor, User } = require('../frames'); +const { hashPassword } = require('../../util/crypto'); const { unjoiner, page, equals, QueryOptions } = require('../../util/db'); const { reject } = require('../../util/promise'); const Problem = require('../../util/problem'); @@ -32,10 +33,10 @@ const update = (user, data) => ({ run, one }) => { update.audit = (user, data) => (log) => log('user.update', user.actor, { data: data.with(data.actor) }); -const updatePassword = (user, cleartext) => ({ run, bcrypt }) => +const updatePassword = (user, cleartext) => ({ run }) => (cleartext.length < 10 ? reject(Problem.user.passwordTooShort()) - : bcrypt.hash(cleartext)) + : hashPassword(cleartext)) .then((hash) => run(sql`update users set password=${hash} where "actorId"=${user.actor.id}`)); updatePassword.audit = (user) => (log) => log('user.update', user.actor, { data: { password: true } }); diff --git a/lib/resources/sessions.js b/lib/resources/sessions.js index 859ee563d..d60ce33f4 100644 --- a/lib/resources/sessions.js +++ b/lib/resources/sessions.js @@ -7,6 +7,7 @@ // including this file, may be copied, modified, propagated, or distributed // except according to the terms contained in the LICENSE file. +const { verifyPassword } = require('../util/crypto'); const Problem = require('../util/problem'); const { isBlank, noargs } = require('../util/util'); const { getOrReject, rejectIf } = require('../util/promise'); @@ -17,9 +18,9 @@ const oidc = require('../util/oidc'); module.exports = (service, endpoint) => { if (!oidc.isEnabled()) { - service.post('/sessions', endpoint(({ Audits, Users, Sessions, bcrypt }, { body, headers }) => { + service.post('/sessions', endpoint(({ Audits, Users, Sessions }, { body, headers }) => { // TODO if we're planning to offer multiple authN methods, we should be looking for - // any calls to bcrypt.verify(), and blocking them if that authN method is not + // any calls to verifyPassword(), and blocking them if that authN method is not // appropriate for the current user. // // It may be useful to re-use the sessions resources for other authN methods. @@ -31,7 +32,7 @@ module.exports = (service, endpoint) => { return Users.getByEmail(email) .then(getOrReject(Problem.user.authenticationFailed())) - .then((user) => bcrypt.verify(password, user.password) + .then((user) => verifyPassword(password, user.password) .then(rejectIf( (verified) => (verified !== true), noargs(Problem.user.authenticationFailed) diff --git a/lib/resources/users.js b/lib/resources/users.js index 468ff3990..91477effa 100644 --- a/lib/resources/users.js +++ b/lib/resources/users.js @@ -9,6 +9,7 @@ const { always } = require('ramda'); const { User } = require('../model/frames'); +const { verifyPassword } = require('../util/crypto'); const { success, isTrue } = require('../util/http'); const Option = require('../util/option'); const Problem = require('../util/problem'); @@ -90,10 +91,10 @@ module.exports = (service, endpoint) => { // TODO: infosec debate around 404 vs 403 if insufficient privs but record DNE. // TODO: exact endpoint naming. - service.put('/users/:id/password', endpoint(async ({ Sessions, Users, mail, bcrypt }, { params, body, auth }) => { + service.put('/users/:id/password', endpoint(async ({ Sessions, Users, mail }, { params, body, auth }) => { const user = await Users.getByActorId(params.id).then(getOrNotFound); await auth.canOrReject('user.update', user.actor); - const verified = await bcrypt.verify(body.old, user.password); + const verified = await verifyPassword(body.old, user.password); if (verified !== true) return Problem.user.authenticationFailed(); await Promise.all([ Users.updatePassword(user, body.new), diff --git a/lib/task/task.js b/lib/task/task.js index 3456b6091..8e1230e84 100644 --- a/lib/task/task.js +++ b/lib/task/task.js @@ -26,7 +26,6 @@ const { serialize } = require('../util/http'); // initialize modules we need to put in the container: -const bcrypt = require('../util/crypto').password(require('bcrypt')); const env = config.get('default.env'); const { mailer } = require('../external/mail'); const mail = mailer(mergeRight(config.get('default.email'), { env })); @@ -47,7 +46,7 @@ const task = { // not thread-safe! but we don't have threads.. withContainer: (taskdef) => (...args) => { const needsContainer = (task._container == null); - if (needsContainer) task._container = container.withDefaults({ db: slonikPool(config.get('default.database')), bcrypt, env, mail, task: true, odkAnalytics }); + if (needsContainer) task._container = container.withDefaults({ db: slonikPool(config.get('default.database')), env, mail, task: true, odkAnalytics }); const result = taskdef(task._container)(...args); diff --git a/lib/util/crypto.js b/lib/util/crypto.js index 577fc1991..a4a259c94 100644 --- a/lib/util/crypto.js +++ b/lib/util/crypto.js @@ -10,6 +10,7 @@ // Various useful cryptography functions, for doing things like hashing passwords, // generating random tokens, and encrypting/decrypting data. +const bcrypt = require('bcrypt'); const digest = require('digest-stream'); const { createHash, randomBytes, generateKeyPair, pbkdf2, createPrivateKey, createPublicKey, createCipheriv, createDecipheriv, publicEncrypt, privateDecrypt } = require('crypto'); const { RSA_NO_PADDING } = require('crypto').constants; @@ -25,13 +26,17 @@ const { isBlank } = require('./util'); //////////////////////////////////////////////////////////////////////////////// // PASSWORD/AUTH UTIL +// In tests, allow an insecure cost-factor for bcrypt. In fact the minimum salt +// is 4, but this is only documented in the bcrypt lib's issue tracker. +// See: https://github.com/kelektiv/node.bcrypt.js/issues/870 +const BCRYPT_COST_FACTOR = process.env.BCRYPT === 'insecure' ? 1 : 12; + // These functions call into bcrypt to hash or verify passwords. -const hashPassword = (bcrypt) => (plain) => - (isBlank(plain) ? resolve(null) : bcrypt.hash(plain, 12)); -const verifyPassword = (bcrypt) => (plain, hash) => ((isBlank(plain) || (isBlank(hash))) +const hashPassword = (plain) => + (isBlank(plain) ? resolve(null) : bcrypt.hash(plain, BCRYPT_COST_FACTOR)); +const verifyPassword = (plain, hash) => ((isBlank(plain) || (isBlank(hash))) ? resolve(false) : bcrypt.compare(plain, hash)); -const password = (bcrypt) => ({ hash: hashPassword(bcrypt), verify: verifyPassword(bcrypt) }); // Returns a cryptographically random base64 string of a given length. const generateToken = (bytes = 48) => randomBytes(bytes).toString('base64') @@ -302,7 +307,7 @@ const submissionDecryptor = (keys, passphrases) => module.exports = { - hashPassword, verifyPassword, password, generateToken, + hashPassword, verifyPassword, generateToken, shasum, sha256sum, md5sum, digestWith, getPrivate, generateManagedKey, generateVersionSuffix, stripPemEnvelope, diff --git a/test/integration/fixtures/01-users.js b/test/integration/fixtures/01-users.js index 00729f313..7e8e345c2 100644 --- a/test/integration/fixtures/01-users.js +++ b/test/integration/fixtures/01-users.js @@ -1,8 +1,9 @@ const appRoot = require('app-root-path'); const { User, Actor, Project } = require(appRoot + '/lib/model/frames'); +const { hashPassword } = require(appRoot + '/lib/util/crypto'); const { mapSequential } = require(appRoot + '/test/util/util'); -module.exports = ({ Assignments, Users, Projects, bcrypt }) => { +module.exports = ({ Assignments, Users, Projects }) => { const proj = new Project({ name: 'Default Project' }); const users = [ @@ -13,7 +14,7 @@ module.exports = ({ Assignments, Users, Projects, bcrypt }) => { // hash the passwords, create our three test users, then add grant Alice and Bob their rights. const withPasswords = Promise.all(users.map((user) => - bcrypt.hash(user.password).then((password) => user.with({ password })))); + hashPassword(user.password).then((password) => user.with({ password })))); return Projects.create(proj) .then(() => withPasswords) diff --git a/test/integration/setup.js b/test/integration/setup.js index 49de57b2f..1d19e3678 100644 --- a/test/integration/setup.js +++ b/test/integration/setup.js @@ -33,12 +33,6 @@ const xlsform = require(appRoot + '/test/util/xlsform'); // set up our sentry mock. const Sentry = require(appRoot + '/lib/external/sentry').init(); -// set up our bcrypt module; possibly mock or not based on params. -const _bcrypt = (process.env.BCRYPT === 'no') - ? require('../util/bcrypt-mock') - : require('bcrypt'); -const bcrypt = require(appRoot + '/lib/util/crypto').password(_bcrypt); - // set up our enketo mock. const { reset: resetEnketo, ...enketo } = require(appRoot + '/test/util/enketo'); // Initialize the mock before other setup that uses the mock, then reset the @@ -87,7 +81,7 @@ const initialize = async () => { await migrator.destroy(); } - return withDefaults({ db, bcrypt, context, enketo, env }).transacting(populate); + return withDefaults({ db, context, enketo, env }).transacting(populate); }; // eslint-disable-next-line func-names, space-before-function-paren @@ -143,7 +137,7 @@ const augment = (service) => { // FINAL TEST WRAPPERS -const baseContainer = withDefaults({ db, mail, env, xlsform, bcrypt, enketo, Sentry, odkAnalytics, context }); +const baseContainer = withDefaults({ db, mail, env, xlsform, enketo, Sentry, odkAnalytics, context }); // called to get a service context per request. we do some work to hijack the // transaction system so that each test runs in a single transaction that then diff --git a/test/integration/task/account.js b/test/integration/task/account.js index 3ab383173..a2d1f8bc9 100644 --- a/test/integration/task/account.js +++ b/test/integration/task/account.js @@ -1,6 +1,7 @@ const appRoot = require('app-root-path'); const should = require('should'); const { testTask } = require('../setup'); +const { verifyPassword } = require(appRoot + '/lib/util/crypto'); const { getOrNotFound } = require(appRoot + '/lib/util/promise'); const { createUser, promoteUser, setUserPassword } = require(appRoot + '/lib/task/account'); const { User } = require(appRoot + '/lib/model/frames'); @@ -35,18 +36,18 @@ describe('task: accounts', () => { should(log.details.data.password).equal(null); }))); - it('should set the password if given', testTask(({ Users, bcrypt }) => + it('should set the password if given', testTask(({ Users }) => createUser('testuser@getodk.org', 'aoeuidhtns') .then(() => Users.getByEmail('testuser@getodk.org')) .then(getOrNotFound) - .then((user) => bcrypt.verify('aoeuidhtns', user.password)) + .then((user) => verifyPassword('aoeuidhtns', user.password)) .then((verified) => verified.should.equal(true)))); - it('should not verify a null password', testTask(({ Users, bcrypt }) => + it('should not verify a null password', testTask(({ Users }) => createUser('testuser@getodk.org', null) .then(() => Users.getByEmail('testuser@getodk.org')) .then(getOrNotFound) - .then((user) => bcrypt.verify(null, user.password)) + .then((user) => verifyPassword(null, user.password)) .then((verified) => verified.should.equal(false)))); it('should complain if the password is too short', testTask(() => @@ -85,12 +86,12 @@ describe('task: accounts', () => { }); describe('setUserPassword', () => { - it('should set a user password', testTask(({ Users, bcrypt }) => + it('should set a user password', testTask(({ Users }) => Users.create(User.fromApi({ email: 'testuser@getodk.org', displayName: 'test user' })) .then(() => setUserPassword('testuser@getodk.org', 'aoeuidhtns')) .then(() => Users.getByEmail('testuser@getodk.org')) .then(getOrNotFound) - .then((user) => bcrypt.verify('aoeuidhtns', user.password)) + .then((user) => verifyPassword('aoeuidhtns', user.password)) .then((verified) => verified.should.equal(true)))); it('should complain about a password that is too short', testTask(({ Users }) => diff --git a/test/unit/http/preprocessors.js b/test/unit/http/preprocessors.js index 6aabdff34..8df4a2fbf 100644 --- a/test/unit/http/preprocessors.js +++ b/test/unit/http/preprocessors.js @@ -6,11 +6,10 @@ const preprocessors = require(appRoot + '/lib/http/preprocessors'); const { Context } = require(appRoot + '/lib/http/endpoint'); const { Session, User, Actor } = require(appRoot + '/lib/model/frames'); const { by } = require(appRoot + '/lib/model/query/auth'); +const { hashPassword } = require(appRoot + '/lib/util/crypto'); const Problem = require(appRoot + '/lib/util/problem'); const Option = require(appRoot + '/lib/util/option'); -const bcrypt = require(appRoot + '/lib/util/crypto').password(require('bcrypt')); - describe('preprocessors', () => { // some mock helpers to simplify testing this module in isolation: const Auth = { by: (x) => by(x)({}) }; @@ -115,7 +114,7 @@ describe('preprocessors', () => { it('should fail the request if the Basic auth credentials are not right', () => Promise.resolve(authHandler( - { Auth, Users: mockUsers('alice@getodk.org', 'willnevermatch'), bcrypt }, + { Auth, Users: mockUsers('alice@getodk.org', 'willnevermatch') }, new Context( createRequest({ headers: { Authorization: `Basic ${Buffer.from('alice@getodk.org:alice', 'utf8').toString('base64')}`, @@ -126,9 +125,9 @@ describe('preprocessors', () => { )).should.be.rejectedWith(Problem, { problemCode: 401.2 })); it('should set the appropriate session if valid Basic auth credentials are given @slow', () => - bcrypt.hash('alice').then((hashed) => + hashPassword('alice').then((hashed) => Promise.resolve(authHandler( - { Auth, Users: mockUsers('alice@getodk.org', hashed), bcrypt }, + { Auth, Users: mockUsers('alice@getodk.org', hashed) }, new Context( createRequest({ headers: { Authorization: `Basic ${Buffer.from('alice@getodk.org:alice', 'utf8').toString('base64')}`, diff --git a/test/unit/util/crypto.js b/test/unit/util/crypto.js index d9cb082f2..0bd666241 100644 --- a/test/unit/util/crypto.js +++ b/test/unit/util/crypto.js @@ -2,38 +2,38 @@ const appRoot = require('app-root-path'); const { readFileSync } = require('fs'); const should = require('should'); const streamTest = require('streamtest').v2; -const bcrypt = require('bcrypt'); const crypto = require(appRoot + '/lib/util/crypto'); describe('util/crypto', () => { - describe('hashPassword/verifyPassword @slow', () => { - const password = crypto.password(bcrypt); + describe('hashPassword/verifyPassword', () => { + const { hashPassword, verifyPassword } = crypto; + // we do not actually verify the hashing itself, as: // 1. it is entirely performed by bcrypt, which has is own tests. // 2. bcrypt is intentionally slow, and we would like unit tests to be fast. it('should always return a Promise', () => { - password.hash('').should.be.a.Promise(); - password.hash('password').should.be.a.Promise(); - password.verify('password', 'hashhash').should.be.a.Promise(); + hashPassword('').should.be.a.Promise(); + hashPassword('password').should.be.a.Promise(); + hashPassword('password', 'hashhash').should.be.a.Promise(); }); it('should return a Promise of null given a blank plaintext', (done) => { - password.hash('').then((result) => { + hashPassword('').then((result) => { should(result).equal(null); done(); }); }); it('should not attempt to verify empty plaintext', (done) => { - password.verify('', '$2a$12$hCRUXz/7Hx2iKPLCduvrWugC5Q/j5e3bX9KvaYvaIvg/uvFYEpzSy').then((result) => { + verifyPassword('', '$2a$12$hCRUXz/7Hx2iKPLCduvrWugC5Q/j5e3bX9KvaYvaIvg/uvFYEpzSy').then((result) => { result.should.equal(false); done(); }); }); it('should not attempt to verify empty hash', (done) => { - password.verify('password', '').then((result) => { + verifyPassword('password', '').then((result) => { result.should.equal(false); done(); }); diff --git a/test/util/bcrypt-mock.js b/test/util/bcrypt-mock.js deleted file mode 100644 index a6dc0c175..000000000 --- a/test/util/bcrypt-mock.js +++ /dev/null @@ -1,5 +0,0 @@ -module.exports = { - hash: (x) => Promise.resolve(x), - compare: (x, y) => Promise.resolve(x === y) -}; -