diff --git a/config.js b/config.js index ebbd30a1..da3e5d2e 100755 --- a/config.js +++ b/config.js @@ -132,6 +132,11 @@ var conf = convict({ default: 'test', env: 'DB_AUTH_NAME' }, + hashSecrets: { + doc: 'Whether to hash client secrets', + format: Boolean, + default: false + }, saltRounds: { doc: 'The number of rounds to go through when hashing a password', format: Number, diff --git a/dadi/lib/model/acl/client.js b/dadi/lib/model/acl/client.js index c2f120c0..b1d49083 100644 --- a/dadi/lib/model/acl/client.js +++ b/dadi/lib/model/acl/client.js @@ -89,9 +89,8 @@ Client.prototype.create = function (client, { ) } - return this.hashSecret(client.secret) + return this.hashSecret(client.secret, client) }).then(hashedsecret => { - client._hashVersion = HASH_VERSION client.secret = hashedsecret return this.model.create({ @@ -198,14 +197,24 @@ Client.prototype.get = function (clientId, secret) { } /** - * Generates a hash from a secret. + * Generates a hash from a secret. If `target` is supplied, a `_hashVersion_` + * property will be added to that object. * * @param {String} secret + * @param {Object} target * @return {Promise} */ -Client.prototype.hashSecret = function (secret) { +Client.prototype.hashSecret = function (secret, target) { + if (!config.get('auth.hashSecrets')) { + return Promise.resolve(secret) + } + const saltRounds = config.get('auth.saltRounds') + if (target) { + target._hashVersion = HASH_VERSION + } + return bcrypt.hash(secret, saltRounds) } @@ -602,8 +611,7 @@ Client.prototype.update = function (clientId, update) { // If we're trying to update the client's secret, we must hash it // before sending it to the database. if (typeof secret === 'string') { - return this.hashSecret(secret).then(hashedSecret => { - update._hashVersion = HASH_VERSION + return this.hashSecret(secret, update).then(hashedSecret => { update.secret = hashedSecret return results @@ -705,6 +713,10 @@ Client.prototype.validate = function (client, { * @return {Promise} */ Client.prototype.validateSecret = function (hash, candidate, hashVersion) { + if (!config.get('auth.hashSecrets')) { + return Promise.resolve(hash === candidate) + } + if (hashVersion !== HASH_VERSION) { return Promise.reject( new Error('CLIENT_NEEDS_UPGRADE') diff --git a/test/acceptance/acl/clients-api/post.js b/test/acceptance/acl/clients-api/post.js index 4345d842..1e4de67d 100644 --- a/test/acceptance/acl/clients-api/post.js +++ b/test/acceptance/acl/clients-api/post.js @@ -524,76 +524,80 @@ module.exports = () => { }) describe('success states (the client has "create" access to the "clients" resource)', () => { - it('should hash client secrets and salt them using the number of rounds specified in the `auth.saltRounds` config property', done => { - config.set('auth.saltRounds', 5) - - const spy = sinon.spy(bcrypt, 'hash') - const testClient = { - clientId: 'apiClient', - secret: 'someSecret', - resources: { - clients: { - create: true + describe('if `auth.hashSecrets` is set to true', () => { + it('should hash client secrets and salt them using the number of rounds specified in the `auth.saltRounds` config property', done => { + config.set('auth.hashSecrets', true) + config.set('auth.saltRounds', 5) + + const spy = sinon.spy(bcrypt, 'hash') + const testClient = { + clientId: 'apiClient', + secret: 'someSecret', + resources: { + clients: { + create: true + } } } - } - const newClient1 = { - clientId: 'newClient1', - secret: 'aNewSecret1' - } - const newClient2 = { - clientId: 'newClient2', - secret: 'aNewSecret2' - } - - help.createACLClient(testClient).then(() => { - client - .post(config.get('auth.tokenUrl')) - .set('content-type', 'application/json') - .send({ - clientId: testClient.clientId, - secret: testClient.secret - }) - .expect(200) - .expect('content-type', 'application/json') - .end((err, res) => { - if (err) return done(err) - - const {accessToken} = res.body - + const newClient1 = { + clientId: 'newClient1', + secret: 'aNewSecret1' + } + const newClient2 = { + clientId: 'newClient2', + secret: 'aNewSecret2' + } + + help.createACLClient(testClient).then(() => { client - .post('/api/clients') - .send(newClient1) + .post(config.get('auth.tokenUrl')) .set('content-type', 'application/json') - .set('Authorization', `Bearer ${accessToken}`) + .send({ + clientId: testClient.clientId, + secret: testClient.secret + }) + .expect(200) .expect('content-type', 'application/json') .end((err, res) => { - res.statusCode.should.eql(201) - - spy.getCall(0).args[0].should.eql('aNewSecret1') - spy.getCall(0).args[1].should.eql(5) - - config.set('auth.saltRounds', 8) - + if (err) return done(err) + + const {accessToken} = res.body + client .post('/api/clients') - .send(newClient2) + .send(newClient1) .set('content-type', 'application/json') .set('Authorization', `Bearer ${accessToken}`) .expect('content-type', 'application/json') .end((err, res) => { res.statusCode.should.eql(201) - config.set('auth.saltRounds', configBackup.auth.saltRounds) - - spy.getCall(1).args[0].should.eql('aNewSecret2') - spy.getCall(1).args[1].should.eql(8) + spy.getCall(0).args[0].should.eql('aNewSecret1') + spy.getCall(0).args[1].should.eql(5) - spy.restore() + config.set('auth.saltRounds', 8) - done(err) - }) - }) + client + .post('/api/clients') + .send(newClient2) + .set('content-type', 'application/json') + .set('Authorization', `Bearer ${accessToken}`) + .expect('content-type', 'application/json') + .end((err, res) => { + res.statusCode.should.eql(201) + + config.set('auth.hashSecrets', configBackup.auth.hashSecrets) + config.set('auth.saltRounds', configBackup.auth.saltRounds) + + spy.getCall(1).args[0].should.eql('aNewSecret2') + spy.getCall(1).args[1].should.eql(8) + + spy.restore() + + done(err) + }) + }) + }) }) }) }) diff --git a/test/acceptance/acl/clients-api/put.js b/test/acceptance/acl/clients-api/put.js index 977b69b7..4b5bcc10 100644 --- a/test/acceptance/acl/clients-api/put.js +++ b/test/acceptance/acl/clients-api/put.js @@ -550,77 +550,83 @@ module.exports = () => { describe('success states', () => { describe('updating the secret', () => { - it('should hash the new secret and salt it using the number of rounds specified in the `auth.saltRounds` config property', done => { - let testClient = { - clientId: 'apiClient', - secret: 'someSecret' - } - - help.createACLClient(testClient).then(() => { - client - .post(config.get('auth.tokenUrl')) - .set('content-type', 'application/json') - .send({ - clientId: testClient.clientId, - secret: testClient.secret - }) - .expect(200) - .expect('content-type', 'application/json') - .end((err, res) => { - if (err) return done(err) - - res.body.accessToken.should.be.String - - let bearerToken = res.body.accessToken - - config.set('auth.saltRounds', 9) - - const spy = sinon.spy(bcrypt, 'hash') - const update = { - currentSecret: 'someSecret', - secret: 'aNewSecret' - } + describe('if `auth.hashSecrets` is set to true', () => { + it('should hash the new secret and salt it using the number of rounds specified in the `auth.saltRounds` config property', done => { + config.set('auth.hashSecrets', true) + let testClient = { + clientId: 'apiClient', + secret: 'someSecret' + } + + help.createACLClient(testClient).then(() => { client - .put('/api/client') - .send(update) + .post(config.get('auth.tokenUrl')) .set('content-type', 'application/json') - .set('Authorization', `Bearer ${bearerToken}`) + .send({ + clientId: testClient.clientId, + secret: testClient.secret + }) + .expect(200) .expect('content-type', 'application/json') .end((err, res) => { - res.statusCode.should.eql(200) - - res.body.results.should.be.Array - res.body.results.length.should.eql(1) - res.body.results[0].clientId.should.eql(testClient.clientId) - - spy.getCall(0).args[0].should.eql(update.secret) - spy.getCall(0).args[1].should.eql(9) - spy.restore() - - config.set('auth.saltRounds', configBackup.auth.saltRounds) - - should.not.exist(res.body.results[0].secret) - + if (err) return done(err) + + res.body.accessToken.should.be.String + + let bearerToken = res.body.accessToken + + config.set('auth.saltRounds', 9) + + const spy = sinon.spy(bcrypt, 'hash') + const update = { + currentSecret: 'someSecret', + secret: 'aNewSecret' + } + client - .post(config.get('auth.tokenUrl')) + .put('/api/client') + .send(update) .set('content-type', 'application/json') - .send({ - clientId: testClient.clientId, - secret: 'aNewSecret' - }) - .expect(200) + .set('Authorization', `Bearer ${bearerToken}`) .expect('content-type', 'application/json') .end((err, res) => { - if (err) return done(err) - - res.body.accessToken.should.be.String - - done() + res.statusCode.should.eql(200) + + res.body.results.should.be.Array + res.body.results.length.should.eql(1) + res.body.results[0].clientId.should.eql(testClient.clientId) + + spy.getCall(0).args[0].should.eql(update.secret) + spy.getCall(0).args[1].should.eql(9) + spy.restore() + + config.set('auth.saltRounds', configBackup.auth.saltRounds) + + should.not.exist(res.body.results[0].secret) + + client + .post(config.get('auth.tokenUrl')) + .set('content-type', 'application/json') + .send({ + clientId: testClient.clientId, + secret: 'aNewSecret' + }) + .expect(200) + .expect('content-type', 'application/json') + .end((err, res) => { + if (err) return done(err) + + config.set('auth.hashSecrets', configBackup.auth.hashSecrets) + + res.body.accessToken.should.be.String + + done() + }) }) }) }) - }) + }) }) it('should allow a client to update their own secret on /api/clients/{ID}', done => { diff --git a/test/acceptance/acl/token-store.js b/test/acceptance/acl/token-store.js index 0880e5fe..738f067a 100644 --- a/test/acceptance/acl/token-store.js +++ b/test/acceptance/acl/token-store.js @@ -59,6 +59,8 @@ describe('Token store', () => { }) it('should return 401 with a specific error message in the www-authenticate header if the client must be upgraded due to an outdated hashing algorithm', done => { + config.set('auth.hashSecrets', true) + const unhashedClient = { clientId: 'anotherTestClient', secret: 'iShouldBeHashed', @@ -80,6 +82,8 @@ describe('Token store', () => { res.headers['www-authenticate'].includes('error="client_needs_upgrade"').should.eql(true) res.headers['www-authenticate'].includes('error_description="The client record on the server must be upgraded"').should.eql(true) + config.set('auth.hashSecrets', configBackup.auth.hashSecrets) + done(err) }) }) @@ -325,41 +329,47 @@ describe('Token store', () => { }) }) - it('should attach client data to the request object if the bearer token supplied is valid', done => { - const newClient = { - clientId: 'testClient1', - secret: 'superSecret1', - accessType: 'admin' - } - const spy = sinon.spy(bcrypt, 'compare') - - help.createClient(newClient, (err, clientRecord) => { - client - .post(tokenRoute) - .send({ - clientId: newClient.clientId, - secret: newClient.secret - }) - .expect('content-type', 'application/json') - .expect('pragma', 'no-cache') - .expect('Cache-Control', 'no-store') - .expect(200, (err, res) => { - res.body.accessToken.should.be.String - - spy.getCall(0).args[0].should.eql(newClient.secret) - spy.getCall(0).args[1].should.eql(clientRecord.secret) - spy.restore() + describe('if `auth.hashSecrets` is set to true', () => { + it('should compare the secret supplied against the one store using a cryptographic compare function', done => { + config.set('auth.hashSecrets', true) + const newClient = { + clientId: 'testClient1', + secret: 'superSecret1', + accessType: 'admin' + } + const spy = sinon.spy(bcrypt, 'compare') + + help.createClient(newClient, (err, clientRecord) => { client - .get('/v1/intercept-client') - .set('Authorization', `Bearer ${res.body.accessToken}`) - .expect('content-type', 'application/json') - .expect(200, (err, res) => { - res.body.clientId.should.eql(newClient.clientId) - res.body.accessType.should.eql(newClient.accessType) - - done() - }) + .post(tokenRoute) + .send({ + clientId: newClient.clientId, + secret: newClient.secret + }) + .expect('content-type', 'application/json') + .expect('pragma', 'no-cache') + .expect('Cache-Control', 'no-store') + .expect(200, (err, res) => { + res.body.accessToken.should.be.String + + spy.getCall(0).args[0].should.eql(newClient.secret) + spy.getCall(0).args[1].should.eql(clientRecord.secret) + spy.restore() + + client + .get('/v1/intercept-client') + .set('Authorization', `Bearer ${res.body.accessToken}`) + .expect('content-type', 'application/json') + .expect(200, (err, res) => { + res.body.clientId.should.eql(newClient.clientId) + res.body.accessType.should.eql(newClient.accessType) + + config.set('auth.hashSecrets', configBackup.auth.hashSecrets) + + done() + }) + }) }) }) }) diff --git a/test/acceptance/help.js b/test/acceptance/help.js index 6dd8e99e..53351b2a 100755 --- a/test/acceptance/help.js +++ b/test/acceptance/help.js @@ -8,15 +8,18 @@ const config = require(__dirname + '/../../config') const request = require('supertest') function hashClientSecret(client) { + if (client._hashVersion === undefined && config.get('auth.hashSecrets')) { + client._hashVersion = 1 + } + switch (client._hashVersion) { - case undefined: case 1: return Object.assign({}, client, { _hashVersion: 1, secret: bcrypt.hashSync(client.secret, config.get('auth.saltRounds')) }) - case null: + default: delete client._hashVersion return client