Merge pull request #189 from screwdriver-cd/revert-token-changes

Revert token changes
screwdriver-cd · Aug 24, 2017 · 6bee2a0 · 6bee2a0
2 parents 14fc87e + e6987a4
commit 6bee2a0
Show file tree

Hide file tree

Showing 6 changed files with 21 additions and 47 deletions.
diff --git a/lib/generateToken.js b/lib/generateToken.js
@@ -1,38 +1,28 @@
 'use strict';
 
-const base64url = require('base64url');
-const crypto = require('crypto');
 const nodeify = require('./nodeify');
-
-// Config for pbkdf2
-// 100,000 iterations is still fast enough to be performant (84ms to run the unit test) while also
-// creating enough of a delay to stop brute force attacks
-// For more info, see https://www.owasp.org/index.php/Password_Storage_Cheat_Sheet
+const crypto = require('crypto');
+const base64url = require('base64url');
 const ALGORITHM = 'sha256';
-const ITERATIONS = 100000;
-
-// Token length, measured in bytes
-const TOKEN_LENGTH = 32;
+const TOKEN_LENGTH = 256; // Measured in bits
 
 /**
  * Create a token value with crypto
  * @method generateValue
  * @return {Promise}
  */
 function generateValue() {
-    return nodeify.withContext(crypto, 'randomBytes', [TOKEN_LENGTH])
+    return nodeify.withContext(crypto, 'randomBytes', [TOKEN_LENGTH / 8])
         .then(base64url);
 }
 
 /**
  * Use crypto to hash a token value
  * @param  {String} value   Token to hash
- * @param  {String} salt    Salt value for PBKDF2
- * @return {Promise}
+ * @return {String}         Hashed value
  */
-function hashValue(value, salt) {
-    return nodeify.withContext(crypto, 'pbkdf2', [value, salt, ITERATIONS, TOKEN_LENGTH, ALGORITHM])
-        .then(base64url);
+function hashValue(value) {
+    return base64url(crypto.createHash(ALGORITHM).update(value).digest());
 }
 
 module.exports = {

diff --git a/lib/token.js b/lib/token.js
@@ -3,8 +3,6 @@
 const BaseModel = require('./base');
 const generateToken = require('./generateToken');
 
-const password = Symbol('password');
-
 class TokenModel extends BaseModel {
     /**
      * Construct a TokenModel object
@@ -16,11 +14,9 @@ class TokenModel extends BaseModel {
      * @param  {String}    config.name              The token name
      * @param  {String}    config.description       The token description
      * @param  {String}    config.lastUsed          The last time the token was used (ISO String)
-     * @param  {String}    config.password          Password used as a salt by PBKDF2
      */
     constructor(config) {
         super('token', config);
-        this[password] = config.password;
     }
 
     /**
@@ -34,7 +30,7 @@ class TokenModel extends BaseModel {
         return generateToken.generateValue()
             .then((bytes) => {
                 value = bytes;
-                this.hash = generateToken.hashValue(bytes, this[password]);
+                this.hash = generateToken.hashValue(bytes);
 
                 return this.update();
             }).then((model) => {

diff --git a/lib/tokenFactory.js b/lib/tokenFactory.js
@@ -4,8 +4,6 @@ const BaseFactory = require('./baseFactory');
 const Token = require('./token');
 const generateToken = require('./generateToken');
 
-const password = Symbol('password');
-
 let instance;
 
 class TokenFactory extends BaseFactory {
@@ -14,11 +12,9 @@ class TokenFactory extends BaseFactory {
      * @method constructor
      * @param  {Object}    config
      * @param  {Object}    config.datastore     Object that will perform operations on the datastore
-     * @param  {String}    config.password      Password used as a salt by PBKDF2
      */
     constructor(config) {
         super('token', config);
-        this[password] = config.password;
     }
 
     /**
@@ -28,11 +24,7 @@ class TokenFactory extends BaseFactory {
      * @return {Token}
      */
     createClass(config) {
-        const c = config;
-
-        c.password = this[password];
-
-        return new Token(c);
+        return new Token(config);
     }
 
     /**
@@ -50,7 +42,7 @@ class TokenFactory extends BaseFactory {
         return generateToken.generateValue()
             .then((bytes) => {
                 value = bytes;
-                config.hash = generateToken.hashValue(bytes, this[password]);
+                config.hash = generateToken.hashValue(bytes);
                 config.lastUsed = '';
 
                 return super.create(config);
@@ -70,7 +62,7 @@ class TokenFactory extends BaseFactory {
      */
     get(config) {
         if (config.value) {
-            config.hash = generateToken.hashValue(config.value, this[password]);
+            config.hash = generateToken.hashValue(config.value);
             delete config.value;
         }
 

diff --git a/test/lib/generateToken.test.js b/test/lib/generateToken.test.js
@@ -9,7 +9,7 @@ sinon.assert.expose(assert, { prefix: '' });
 describe('generateToken', () => {
     const RANDOM_BYTES = 'some random bytes';
     // Result of passing 'some random bytes' through a sha256 hash, in base64
-    const expectedHash = 'qGFuuDVptDRID0ZAOjv8ZVXfT4QfXQzVKc_wyCZKEVg';
+    const HASH = 'mF0EvjvxWMrVz5ZGJcnbe0ZPooUlv_DAB9VrV6bmZmg';
     let firstValue;
 
     it('generates a value', () =>
@@ -27,12 +27,11 @@ describe('generateToken', () => {
             });
     });
 
-    it('hashes a value', () => generateToken.hashValue(RANDOM_BYTES, '')
-        .then((hash) => {
-            assert.strictEqual(hash, expectedHash);
-        }));
+    it('hashes a value', () => {
+        assert.strictEqual(generateToken.hashValue(RANDOM_BYTES), HASH);
+    });
 
     it('hashes a different value to a different hash', () => {
-        assert.notEqual(generateToken.hashValue('some different bytes', ''), expectedHash);
+        assert.notEqual(generateToken.hashValue('some different bytes'), HASH);
     });
 });
diff --git a/test/lib/token.test.js b/test/lib/token.test.js
@@ -8,7 +8,6 @@ const schema = require('screwdriver-data-schema');
 sinon.assert.expose(assert, { prefix: '' });
 
 describe('Token Model', () => {
-    const password = 'totallySecurePassword';
     let datastore;
     let generateTokenMock;
     let BaseModel;
@@ -47,8 +46,7 @@ describe('Token Model', () => {
             id: 6789,
             name: 'Mobile client auth token',
             description: 'For the mobile app',
-            lastUsed: '2017-05-10T01:49:59.327Z',
-            password
+            lastUsed: '2017-05-10T01:49:59.327Z'
         };
         token = new TokenModel(createConfig);
     });
@@ -96,7 +94,7 @@ describe('Token Model', () => {
             return token.refresh()
                 .then((model) => {
                     assert.calledOnce(generateTokenMock.generateValue);
-                    assert.calledWith(generateTokenMock.hashValue, newValue, password);
+                    assert.calledWith(generateTokenMock.hashValue, newValue);
                     assert.strictEqual(model.value, newValue);
                     assert.strictEqual(model.hash, newHash);
                 });

diff --git a/test/lib/tokenFactory.test.js b/test/lib/tokenFactory.test.js
@@ -7,7 +7,6 @@ const sinon = require('sinon');
 sinon.assert.expose(assert, { prefix: '' });
 
 describe('Token Factory', () => {
-    const password = 'totallySecurePassword';
     const name = 'mobile_token';
     const description = 'a token for a mobile app';
     const userId = 6789;
@@ -62,7 +61,7 @@ describe('Token Factory', () => {
         TokenFactory = require('../../lib/tokenFactory');
         /* eslint-enable global-require */
 
-        factory = new TokenFactory({ datastore, password });
+        factory = new TokenFactory({ datastore });
     });
 
     afterEach(() => {
@@ -76,7 +75,7 @@ describe('Token Factory', () => {
 
     describe('createClass', () => {
         it('should return a Token', () => {
-            const model = factory.createClass(Object.assign({}, tokenData));
+            const model = factory.createClass(tokenData);
 
             assert.instanceOf(model, Token);
         });
@@ -94,7 +93,7 @@ describe('Token Factory', () => {
             }).then((model) => {
                 assert.isTrue(datastore.save.calledOnce);
                 assert.calledOnce(generateTokenMock.generateValue);
-                assert.calledWith(generateTokenMock.hashValue, randomBytes, password);
+                assert.calledWith(generateTokenMock.hashValue, randomBytes);
                 assert.calledWith(datastore.save, {
                     params: expected,
                     table: 'tokens'