-
Notifications
You must be signed in to change notification settings - Fork 80
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
Noobaa Account: Replace bcrypt password hashing by crypto #8252
base: master
Are you sure you want to change the base?
Conversation
As bcrypt is not under active maintenance, we need to replace it with node.js crypto hashing module. Signed-off-by: Ashish Pandey <aspandey@redhat.com>
e19fea0
to
6a9f525
Compare
const CRYPTO_SALT_BUFFER_SIZE = 8; | ||
const CRYPTO_SALT_KEY_LENGTH = 32; | ||
const CRYPTO_PBKDF2_ITERATIONS = 100; | ||
|
||
const crypto = require('crypto'); | ||
|
||
function create_node_password_hash(password) { | ||
return new Promise((resolve, reject) => { | ||
const salt = crypto.randomBytes(CRYPTO_SALT_BUFFER_SIZE).toString('hex'); | ||
crypto.pbkdf2(password, salt, CRYPTO_PBKDF2_ITERATIONS, CRYPTO_SALT_KEY_LENGTH, 'sha512', (err, derivedKey) => { | ||
if (err) { | ||
reject(err); | ||
return err; | ||
} | ||
resolve(salt + ':' + derivedKey.toString('hex')); | ||
}); | ||
}); | ||
} | ||
|
||
function verify_node_password_hash(password, hashedPassword) { | ||
return new Promise((resolve, reject) => { | ||
const [salt, key] = hashedPassword.split(':'); | ||
crypto.pbkdf2(password, salt, CRYPTO_PBKDF2_ITERATIONS, CRYPTO_SALT_KEY_LENGTH, 'sha512', (err, derivedKey) => { | ||
if (err) { | ||
reject(err); | ||
return err; | ||
} | ||
resolve(key === derivedKey.toString('hex')); | ||
}); | ||
}); | ||
} | ||
|
||
|
||
exports.create_node_password_hash = create_node_password_hash; | ||
exports.verify_node_password_hash = verify_node_password_hash; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would call this new module password_utils
and implement it with async await:
const CRYPTO_SALT_BUFFER_SIZE = 8; | |
const CRYPTO_SALT_KEY_LENGTH = 32; | |
const CRYPTO_PBKDF2_ITERATIONS = 100; | |
const crypto = require('crypto'); | |
function create_node_password_hash(password) { | |
return new Promise((resolve, reject) => { | |
const salt = crypto.randomBytes(CRYPTO_SALT_BUFFER_SIZE).toString('hex'); | |
crypto.pbkdf2(password, salt, CRYPTO_PBKDF2_ITERATIONS, CRYPTO_SALT_KEY_LENGTH, 'sha512', (err, derivedKey) => { | |
if (err) { | |
reject(err); | |
return err; | |
} | |
resolve(salt + ':' + derivedKey.toString('hex')); | |
}); | |
}); | |
} | |
function verify_node_password_hash(password, hashedPassword) { | |
return new Promise((resolve, reject) => { | |
const [salt, key] = hashedPassword.split(':'); | |
crypto.pbkdf2(password, salt, CRYPTO_PBKDF2_ITERATIONS, CRYPTO_SALT_KEY_LENGTH, 'sha512', (err, derivedKey) => { | |
if (err) { | |
reject(err); | |
return err; | |
} | |
resolve(key === derivedKey.toString('hex')); | |
}); | |
}); | |
} | |
exports.create_node_password_hash = create_node_password_hash; | |
exports.verify_node_password_hash = verify_node_password_hash; | |
const HASH_PASSWORD_SALT_SIZE = 64; | |
const HASH_PASSWORD_KEY_SIZE = 64; | |
const HASH_PASSWORD_ITERATIONS = 100000; | |
const HASH_PASSWORD_DIGEST = 'sha512'; | |
const util = require('node:util'); | |
const crypto = require('node:crypto'); | |
const pbkdf2 = util.promisify(crypto.pbkdf2); | |
async function derive_key(password, salt) { | |
return pbkdf2(password, salt, HASH_PASSWORD_ITERATIONS, HASH_PASSWORD_KEY_SIZE, HASH_PASSWORD_DIGEST); | |
} | |
async function hash_password(password) { | |
const salt = crypto.randomBytes(HASH_PASSWORD_SALT_SIZE); | |
const key = derive_key(password, salt); | |
return salt.toString('hex') + ':' + key.toString('hex'); | |
} | |
async function hash_compare(password, hash) { | |
const [salt, verify_key] = hash.split(':'); | |
const key = derive_key(password, salt); | |
return crypto.timingSafeEqual(key, Buffer.from(verify_key, 'hex')); | |
} | |
exports.hash_password = hash_password; | |
exports.hash_compare = hash_compare; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not so sure about the choice of numbers - iteration count, salt size, and key size...
I was looking at the nodejs docs - https://nodejs.org/api/crypto.html#cryptopbkdf2password-salt-iterations-keylen-digest-callback
The iterations argument must be a number set as high as possible. The higher the number of iterations, the more secure the derived key will be, but will take a longer amount of time to complete.
The salt should be as unique as possible. It is recommended that a salt is random and at least 16 bytes long. See NIST SP 800-132 for details.
if (bcrypt.compare(password.unwrap(), target_account.password.unwrap())) { | ||
return true; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is this supposed to do - are we still allowing previous bcyrpt?
This needs to be at least explained in a code comment.
@@ -6,7 +6,8 @@ const _ = require('lodash'); | |||
const net = require('net'); | |||
const chance = require('chance')(); | |||
const GoogleStorage = require('../../util/google_storage_wrap'); | |||
const bcrypt = require('bcrypt'); | |||
const node_hash = require('../../util/account_password_hash'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would call this new module password_utils
const node_hash = require('../../util/account_password_hash'); | |
const password_utils = require('../../util/password_utils'); |
function crypto_password(password) { | ||
return P.resolve() | ||
.then(() => password && node_hash.create_node_password_hash(password)); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better get rid of this P.resolve and use proper async function
@@ -21,6 +22,14 @@ const config = require('../../../config'); | |||
const s3_bucket_policy_utils = require('../../endpoint/s3/s3_bucket_policy_utils'); | |||
|
|||
|
|||
|
|||
function compare_password_hash(password, target_account) { | |||
if (bcrypt.compare(password.unwrap(), target_account.password.unwrap())) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is bcrypt.compare a sync function call or async?
This PR had no activity for too long - it will now be labeled stale. Update it to prevent it from getting closed. |
As bcrypt is not under active maintenance, we need to replace it with node.js crypto hashing module.
This PR is in line with #8213
IN this PR I tried to implement in-built node.js crypto module. Over all approach is same.
We just need to implement two addition functions to create hash and verify that hash.