Skip to content
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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

aspandey
Copy link
Contributor

@aspandey aspandey commented Aug 5, 2024

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.

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>
Comment on lines +4 to +38
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;
Copy link
Member

@guymguym guymguym Aug 5, 2024

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:

Suggested change
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;

Copy link
Member

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.

Comment on lines +27 to +29
if (bcrypt.compare(password.unwrap(), target_account.password.unwrap())) {
return true;
}
Copy link
Member

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');
Copy link
Member

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

Suggested change
const node_hash = require('../../util/account_password_hash');
const password_utils = require('../../util/password_utils');

Comment on lines +1362 to 1365
function crypto_password(password) {
return P.resolve()
.then(() => password && node_hash.create_node_password_hash(password));
}
Copy link
Member

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())) {
Copy link
Member

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?

Copy link

github-actions bot commented Nov 3, 2024

This PR had no activity for too long - it will now be labeled stale. Update it to prevent it from getting closed.

@github-actions github-actions bot added the Stale label Nov 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants