Noobaa Account: Replace bcrypt password hashing by crypto #8252

aspandey · 2024-08-05T05:57:47Z

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 <[email protected]>

guymguym · 2024-08-05T09:30:58Z

src/util/account_password_hash.js

+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;


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;

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.

guymguym · 2024-08-05T10:35:15Z

src/server/common_services/auth_server.js

+    if (bcrypt.compare(password.unwrap(), target_account.password.unwrap())) {
+        return true;
+    }


What is this supposed to do - are we still allowing previous bcyrpt?
This needs to be at least explained in a code comment.

guymguym · 2024-08-05T10:36:04Z

src/server/system_services/account_server.js

@@ -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');


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');

guymguym · 2024-08-05T10:37:03Z

src/server/system_services/account_server.js

+function crypto_password(password) {
+        return P.resolve()
+        .then(() => password && node_hash.create_node_password_hash(password));
 }


Better get rid of this P.resolve and use proper async function

guymguym · 2024-08-05T10:38:30Z

src/server/common_services/auth_server.js

@@ -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())) {


is bcrypt.compare a sync function call or async?

github-actions · 2024-11-03T11:00:37Z

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

github-actions · 2024-12-03T11:00:47Z

This PR is stale and had no activity for too long - it will now be closed.

pull-request-size bot added the size/M label Aug 5, 2024

aspandey mentioned this pull request Aug 5, 2024

Noobaa Account: Replace bcrypt password hashing by argon2 #8213

Closed

Noobaa Account: Replace bcrypt password hashing by crypto

6a9f525

As bcrypt is not under active maintenance, we need to replace it with node.js crypto hashing module. Signed-off-by: Ashish Pandey <[email protected]>

aspandey force-pushed the bcrypt-to-node-hash branch from e19fea0 to 6a9f525 Compare August 5, 2024 06:28

guymguym requested changes Aug 5, 2024

View reviewed changes

guymguym mentioned this pull request Aug 6, 2024

Noobaa Account: Remove password hashing #8259

Open

github-actions bot added the Stale label Nov 3, 2024

github-actions bot closed this Dec 3, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Noobaa Account: Replace bcrypt password hashing by crypto #8252

Noobaa Account: Replace bcrypt password hashing by crypto #8252

aspandey commented Aug 5, 2024

guymguym Aug 5, 2024 •

edited

Loading

guymguym Aug 5, 2024

guymguym Aug 5, 2024

guymguym Aug 5, 2024

guymguym Aug 5, 2024

guymguym Aug 5, 2024

github-actions bot commented Nov 3, 2024

github-actions bot commented Dec 3, 2024

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

Noobaa Account: Replace bcrypt password hashing by crypto #8252

Noobaa Account: Replace bcrypt password hashing by crypto #8252

Conversation

aspandey commented Aug 5, 2024

guymguym Aug 5, 2024 • edited Loading

Choose a reason for hiding this comment

guymguym Aug 5, 2024

Choose a reason for hiding this comment

guymguym Aug 5, 2024

Choose a reason for hiding this comment

guymguym Aug 5, 2024

Choose a reason for hiding this comment

guymguym Aug 5, 2024

Choose a reason for hiding this comment

guymguym Aug 5, 2024

Choose a reason for hiding this comment

github-actions bot commented Nov 3, 2024

github-actions bot commented Dec 3, 2024

guymguym Aug 5, 2024 •

edited

Loading