From fb9cc41a4bd099ce1fc81718ef2cd07621aec15c Mon Sep 17 00:00:00 2001 From: Abraham Ingersoll <586805+aberoham@users.noreply.github.com> Date: Sat, 11 Apr 2026 08:26:55 +0100 Subject: [PATCH 1/6] feat: self-describing password hashes (iterations$hex) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Store PBKDF2 iteration count alongside the hash to eliminate the linear fallback chain that tried current then legacy iterations on every login attempt. A wrong password now costs exactly one PBKDF2 computation regardless of how many iteration baselines have existed. Format: `iterations$hexHash` (validated by /^\d+\$[0-9a-f]{64}$/). Legacy formats (plain text, SHA-1, raw PBKDF2-5000) are detected and silently upgraded to self-describing format on login. validPassword() returns { valid, needsUpgrade } instead of setting mutable instance state — callers destructure the result directly. PBKDF2_ITERATIONS env var (default 220000) is validated at module load. Future iteration bumps require zero code changes — users at older iterations upgrade on next login. Co-Authored-By: Claude Opus 4.6 (1M context) --- lib/user/store/base.js | 56 +++++++++++--- lib/user/store/mysql.js | 15 +++- lib/user/store/toml.js | 2 +- lib/user/test/index.js | 166 +++++++++++++++++++++++++++++++++++----- routes/user.js | 2 +- 5 files changed, 209 insertions(+), 32 deletions(-) diff --git a/lib/user/store/base.js b/lib/user/store/base.js index 0cc77ed..1ba6e07 100644 --- a/lib/user/store/base.js +++ b/lib/user/store/base.js @@ -1,5 +1,18 @@ import crypto from 'node:crypto' +const PBKDF2_ITERATIONS = (() => { + const raw = process.env.PBKDF2_ITERATIONS + if (raw === undefined) return 220000 + const parsed = parseInt(raw, 10) + if (Number.isNaN(parsed) || parsed < 1) { + console.warn(`PBKDF2_ITERATIONS="${raw}" is not a valid positive integer, using 220000`) + return 220000 + } + return parsed +})() +const LEGACY_ITERATIONS = 5000 +const SELF_DESCRIBING_RE = /^(\d+)\$([0-9a-f]{64})$/ + /** * User domain class – pure attributes and password business logic. * @@ -64,33 +77,58 @@ class UserBase { return salt } - async hashAuthPbkdf2(pass, salt) { + async hashAuthPbkdf2(pass, salt, iterations = PBKDF2_ITERATIONS) { return new Promise((resolve, reject) => { - // match the defaults for NicTool 2.x - crypto.pbkdf2(pass, salt, 5000, 32, 'sha512', (err, derivedKey) => { + crypto.pbkdf2(pass, salt, iterations, 32, 'sha512', (err, derivedKey) => { if (err) return reject(err) resolve(derivedKey.toString('hex')) }) }) } + async hashForStorage(pass, salt, iterations = PBKDF2_ITERATIONS) { + const hex = await this.hashAuthPbkdf2(pass, salt, iterations) + return `${iterations}$${hex}` + } + async validPassword(passTry, passDb, username, salt) { - if (!salt && passTry === passDb) return true // plain pass, TODO, encrypt! + if (!salt && passTry === passDb) { + return { valid: true, needsUpgrade: true } + } if (salt) { - const hashed = await this.hashAuthPbkdf2(passTry, salt) - if (this.debug) console.log(`hashed: (${hashed === passDb}) ${hashed}`) - return hashed === passDb + // Self-describing format: "iterations$hexHash" — single hash, no fallback + const m = SELF_DESCRIBING_RE.exec(passDb) + if (m) { + const storedIters = parseInt(m[1], 10) + const storedHashHex = m[2] + const hashed = await this.hashAuthPbkdf2(passTry, salt, storedIters) + if (this.debug) console.log(`self-describing: (${hashed === storedHashHex}) ${hashed}`) + if (hashed === storedHashHex) { + return { valid: true, needsUpgrade: storedIters < PBKDF2_ITERATIONS } + } + return { valid: false, needsUpgrade: false } + } + + // Raw hex (legacy NicTool 2 format, implicitly 5000 iterations) + const legacy = await this.hashAuthPbkdf2(passTry, salt, LEGACY_ITERATIONS) + if (this.debug) console.log(`legacy: (${legacy === passDb}) ${legacy}`) + if (legacy === passDb) { + return { valid: true, needsUpgrade: true } + } + return { valid: false, needsUpgrade: false } } // Check for HMAC SHA-1 password if (/^[0-9a-f]{40}$/.test(passDb)) { const digest = crypto.createHmac('sha1', username.toLowerCase()).update(passTry).digest('hex') if (this.debug) console.log(`digest: (${digest === passDb}) ${digest}`) - return digest === passDb + if (digest === passDb) { + return { valid: true, needsUpgrade: true } + } } - return false + return { valid: false, needsUpgrade: false } } } diff --git a/lib/user/store/mysql.js b/lib/user/store/mysql.js index 352dbc1..63181eb 100644 --- a/lib/user/store/mysql.js +++ b/lib/user/store/mysql.js @@ -36,7 +36,18 @@ class UserRepoMySQL extends UserBase { AND g.name = ?` for (const u of await Mysql.execute(query, [username, groupName])) { - if (await this.validPassword(authTry.password, u.password, authTry.username, u.pass_salt)) { + const { valid, needsUpgrade } = await this.validPassword(authTry.password, u.password, authTry.username, u.pass_salt) + if (valid) { + if (needsUpgrade) { + const salt = this.generateSalt() + const hash = await this.hashForStorage(authTry.password, salt) + await Mysql.execute( + ...Mysql.update('nt_user', `nt_user_id=${u.id}`, { + password: hash, + pass_salt: salt, + }), + ) + } for (const f of ['password', 'pass_salt']) { delete u[f] // SECURITY: no longer needed } @@ -65,7 +76,7 @@ class UserRepoMySQL extends UserBase { if (args.password) { if (!args.pass_salt) args.pass_salt = this.generateSalt() - args.password = await this.hashAuthPbkdf2(args.password, args.pass_salt) + args.password = await this.hashForStorage(args.password, args.pass_salt) } const userId = await Mysql.execute(...Mysql.insert(`nt_user`, mapToDbColumn(args, userDbMap))) diff --git a/lib/user/store/toml.js b/lib/user/store/toml.js index dd37043..8a06ec9 100644 --- a/lib/user/store/toml.js +++ b/lib/user/store/toml.js @@ -152,7 +152,7 @@ class UserRepoTOML extends UserBase { if (args.password) { if (!args.pass_salt) args.pass_salt = this.generateSalt() - args.password = await this.hashAuthPbkdf2(args.password, args.pass_salt) + args.password = await this.hashForStorage(args.password, args.pass_salt) } if (inherit === false) { diff --git a/lib/user/test/index.js b/lib/user/test/index.js index b24872b..2499d2c 100644 --- a/lib/user/test/index.js +++ b/lib/user/test/index.js @@ -1,8 +1,10 @@ import assert from 'node:assert/strict' +import crypto from 'node:crypto' import { describe, it, after, before } from 'node:test' import User from '../index.js' import Group from '../../group/index.js' +import Mysql from '../../mysql.js' import userJson from './user.json' with { type: 'json' } import groupJson from '../../group/test/group.json' with { type: 'json' } @@ -91,27 +93,35 @@ describe('user', function () { describe('validPassword', function () { it('auths user with plain text password', async () => { const r = await User.validPassword('test', 'test', 'demo', '') - assert.equal(r, true) + assert.deepEqual(r, { valid: true, needsUpgrade: true }) }) - it('auths valid pbkdb2 password', async () => { - const r = await User.validPassword( - 'YouGuessedIt!', - '050cfa70c3582be0d5bfae25138a8486dc2e6790f39bc0c4e111223ba6034432', - 'unit-test', - '(ICzAm2.QfCa6.MN', - ) - assert.equal(r, true) + it('auths valid self-describing PBKDF2 password', async () => { + const salt = '(ICzAm2.QfCa6.MN' + const hash = await User.hashForStorage('YouGuessedIt!', salt) + const r = await User.validPassword('YouGuessedIt!', hash, 'unit-test', salt) + assert.deepEqual(r, { valid: true, needsUpgrade: false }) }) - it('rejects invalid pbkdb2 password', async () => { - const r = await User.validPassword( - 'YouMissedIt!', - '050cfa70c3582be0d5bfae25138a8486dc2e6790f39bc0c4e111223ba6034432', - 'unit-test', - '(ICzAm2.QfCa6.MN', - ) - assert.equal(r, false) + it('rejects invalid self-describing PBKDF2 password', async () => { + const salt = '(ICzAm2.QfCa6.MN' + const hash = await User.hashForStorage('YouGuessedIt!', salt) + const r = await User.validPassword('YouMissedIt!', hash, 'unit-test', salt) + assert.deepEqual(r, { valid: false, needsUpgrade: false }) + }) + + it('auths valid legacy PBKDF2-5000 password', async () => { + const salt = '(ICzAm2.QfCa6.MN' + const hash = await User.hashAuthPbkdf2('YouGuessedIt!', salt, 5000) + const r = await User.validPassword('YouGuessedIt!', hash, 'unit-test', salt) + assert.deepEqual(r, { valid: true, needsUpgrade: true }) + }) + + it('rejects invalid legacy PBKDF2-5000 password', async () => { + const salt = '(ICzAm2.QfCa6.MN' + const hash = await User.hashAuthPbkdf2('YouGuessedIt!', salt, 5000) + const r = await User.validPassword('YouMissedIt!', hash, 'unit-test', salt) + assert.deepEqual(r, { valid: false, needsUpgrade: false }) }) it('auths valid SHA1 password', async () => { @@ -120,7 +130,7 @@ describe('user', function () { '083007777a5241d01abba70c938c60d80be60027', 'unit-test', ) - assert.equal(r, true) + assert.deepEqual(r, { valid: true, needsUpgrade: true }) }) it('rejects invalid SHA1 password', async () => { @@ -129,7 +139,7 @@ describe('user', function () { '083007777a5241d01abba7Oc938c60d80be60027', 'unit-test', ) - assert.equal(r, false) + assert.deepEqual(r, { valid: false, needsUpgrade: false }) }) }) @@ -158,4 +168,122 @@ describe('user', function () { assert.ok(u) }) }) + + describe('password upgrade on login', () => { + const upgradeUserId = 4200 + const upgradeUser = { + nt_user_id: upgradeUserId, + nt_group_id: groupCase.id, + username: 'upgrade-test', + email: 'upgrade-test@example.com', + first_name: 'Upgrade', + last_name: 'Test', + } + const testPass = 'UpgradeMe!123' + const authCreds = { + username: `${upgradeUser.username}@${groupCase.name}`, + password: testPass, + } + + async function getDbPassword() { + const rows = await Mysql.execute( + 'SELECT password, pass_salt FROM nt_user WHERE nt_user_id = ?', + [upgradeUserId], + ) + return rows[0] + } + + // Raw SQL so we can insert specific legacy password formats (plain text, + // SHA-1, PBKDF2-5000) that User.create() would hash on the way in. + async function insertUser(password, passSalt) { + await Mysql.execute( + 'INSERT INTO nt_user (nt_user_id, nt_group_id, username, email, first_name, last_name, password, pass_salt) VALUES (?, ?, ?, ?, ?, ?, ?, ?)', + [upgradeUserId, upgradeUser.nt_group_id, upgradeUser.username, upgradeUser.email, upgradeUser.first_name, upgradeUser.last_name, password, passSalt], + ) + } + + async function cleanup() { + await Mysql.execute( + 'DELETE FROM nt_user WHERE nt_user_id = ?', + [upgradeUserId], + ) + } + + before(cleanup) + after(cleanup) + + it('upgrades plain text password to self-describing PBKDF2 on login', async () => { + await cleanup() + await insertUser(testPass, null) + + const result = await User.authenticate(authCreds) + assert.ok(result, 'login should succeed') + + const row = await getDbPassword() + assert.ok(row.pass_salt, 'pass_salt should be set after upgrade') + assert.notEqual(row.password, testPass, 'password should be hashed') + assert.ok(row.password.includes('$'), 'password should be in self-describing format') + + // verify round-trip: can still log in with the upgraded hash + const again = await User.authenticate(authCreds) + assert.ok(again, 'login should succeed after upgrade') + await cleanup() + }) + + it('upgrades SHA1 password to self-describing PBKDF2 on login', async () => { + // authenticate() passes the full authTry.username (including @group) to + // validPassword(), so the HMAC key must match that full string + const sha1Hash = crypto + .createHmac('sha1', authCreds.username.toLowerCase()) + .update(testPass) + .digest('hex') + await cleanup() + await insertUser(sha1Hash, null) + + const result = await User.authenticate(authCreds) + assert.ok(result, 'login should succeed with SHA1 hash') + + const row = await getDbPassword() + assert.ok(row.pass_salt, 'pass_salt should be set after upgrade') + assert.notEqual(row.password, sha1Hash, 'password should be re-hashed') + assert.ok(row.password.includes('$'), 'password should be in self-describing format') + + const again = await User.authenticate(authCreds) + assert.ok(again, 'login should succeed after upgrade') + await cleanup() + }) + + it('upgrades PBKDF2-5000 to self-describing format on login', async () => { + const legacySalt = User.generateSalt() + const legacyHash = await User.hashAuthPbkdf2(testPass, legacySalt, 5000) + await cleanup() + await insertUser(legacyHash, legacySalt) + + const result = await User.authenticate(authCreds) + assert.ok(result, 'login should succeed with legacy PBKDF2') + + const row = await getDbPassword() + assert.notEqual(row.password, legacyHash, 'password should be re-hashed') + assert.notEqual(row.pass_salt, legacySalt, 'salt should be regenerated') + assert.ok(row.password.includes('$'), 'password should be in self-describing format') + + const again = await User.authenticate(authCreds) + assert.ok(again, 'login should succeed after upgrade') + await cleanup() + }) + + it('does not re-hash password already in self-describing format', async () => { + const salt = User.generateSalt() + const hash = await User.hashForStorage(testPass, salt) + await cleanup() + await insertUser(hash, salt) + + await User.authenticate(authCreds) + + const row = await getDbPassword() + assert.equal(row.password, hash, 'password should be unchanged') + assert.equal(row.pass_salt, salt, 'salt should be unchanged') + await cleanup() + }) + }) }) diff --git a/routes/user.js b/routes/user.js index 85f7b6e..5216c43 100644 --- a/routes/user.js +++ b/routes/user.js @@ -136,7 +136,7 @@ function UserRoutes(server) { if (args.password) { args.pass_salt = User.generateSalt() - args.password = await User.hashAuthPbkdf2(args.password, args.pass_salt) + args.password = await User.hashForStorage(args.password, args.pass_salt) } await User.put(args) From abc9bd4dd2089e20328b8ed83085742fe581614f Mon Sep 17 00:00:00 2001 From: Matt Simerson Date: Sat, 25 Jul 2026 14:11:13 -0700 Subject: [PATCH 2/6] bound the iterations --- lib/user/store/base.js | 44 +++++++++++++++++++++++++++++++---------- lib/user/store/mysql.js | 29 ++++++++++++++++++--------- lib/user/store/toml.js | 21 +++++++++++++++++++- lib/user/test/index.js | 30 ++++++++++++++++++---------- 4 files changed, 94 insertions(+), 30 deletions(-) diff --git a/lib/user/store/base.js b/lib/user/store/base.js index 1ba6e07..7accf91 100644 --- a/lib/user/store/base.js +++ b/lib/user/store/base.js @@ -13,6 +13,16 @@ const PBKDF2_ITERATIONS = (() => { const LEGACY_ITERATIONS = 5000 const SELF_DESCRIBING_RE = /^(\d+)\$([0-9a-f]{64})$/ +// a stored iteration count is attacker-influenced if the DB is tampered with +const MAX_STORED_ITERATIONS = 10_000_000 + +function equalsConstantTime(a, b) { + const bufA = Buffer.from(a, 'hex') + const bufB = Buffer.from(b, 'hex') + if (bufA.length !== bufB.length) return false + return crypto.timingSafeEqual(bufA, bufB) +} + /** * User domain class – pure attributes and password business logic. * @@ -92,6 +102,8 @@ class UserBase { } async validPassword(passTry, passDb, username, salt) { + const invalid = { valid: false, needsUpgrade: false } + if (!salt && passTry === passDb) { return { valid: true, needsUpgrade: true } } @@ -102,33 +114,45 @@ class UserBase { if (m) { const storedIters = parseInt(m[1], 10) const storedHashHex = m[2] - const hashed = await this.hashAuthPbkdf2(passTry, salt, storedIters) - if (this.debug) console.log(`self-describing: (${hashed === storedHashHex}) ${hashed}`) - if (hashed === storedHashHex) { + if (storedIters < 1 || storedIters > MAX_STORED_ITERATIONS) { + console.warn(`refusing stored PBKDF2 iteration count ${storedIters} for user ${username}`) + return invalid + } + + let hashed + try { + hashed = await this.hashAuthPbkdf2(passTry, salt, storedIters) + } catch (err) { + console.error(`PBKDF2 failed for user ${username}`, err) + return invalid + } + + if (this.debug) console.log(`self-describing: ${hashed}`) + if (equalsConstantTime(hashed, storedHashHex)) { return { valid: true, needsUpgrade: storedIters < PBKDF2_ITERATIONS } } - return { valid: false, needsUpgrade: false } + return invalid } // Raw hex (legacy NicTool 2 format, implicitly 5000 iterations) const legacy = await this.hashAuthPbkdf2(passTry, salt, LEGACY_ITERATIONS) - if (this.debug) console.log(`legacy: (${legacy === passDb}) ${legacy}`) - if (legacy === passDb) { + if (this.debug) console.log(`legacy: ${legacy}`) + if (/^[0-9a-f]{64}$/.test(passDb) && equalsConstantTime(legacy, passDb)) { return { valid: true, needsUpgrade: true } } - return { valid: false, needsUpgrade: false } + return invalid } // Check for HMAC SHA-1 password if (/^[0-9a-f]{40}$/.test(passDb)) { const digest = crypto.createHmac('sha1', username.toLowerCase()).update(passTry).digest('hex') - if (this.debug) console.log(`digest: (${digest === passDb}) ${digest}`) - if (digest === passDb) { + if (this.debug) console.log(`digest: ${digest}`) + if (equalsConstantTime(digest, passDb)) { return { valid: true, needsUpgrade: true } } } - return { valid: false, needsUpgrade: false } + return invalid } } diff --git a/lib/user/store/mysql.js b/lib/user/store/mysql.js index 63181eb..53d1a49 100644 --- a/lib/user/store/mysql.js +++ b/lib/user/store/mysql.js @@ -36,17 +36,28 @@ class UserRepoMySQL extends UserBase { AND g.name = ?` for (const u of await Mysql.execute(query, [username, groupName])) { - const { valid, needsUpgrade } = await this.validPassword(authTry.password, u.password, authTry.username, u.pass_salt) + const { valid, needsUpgrade } = await this.validPassword( + authTry.password, + u.password, + authTry.username, + u.pass_salt, + ) if (valid) { + // best effort: a failed rewrite must not cost the user their login, + // they simply get upgraded on a later attempt if (needsUpgrade) { - const salt = this.generateSalt() - const hash = await this.hashForStorage(authTry.password, salt) - await Mysql.execute( - ...Mysql.update('nt_user', `nt_user_id=${u.id}`, { - password: hash, - pass_salt: salt, - }), - ) + try { + const salt = this.generateSalt() + const hash = await this.hashForStorage(authTry.password, salt) + await Mysql.execute( + ...Mysql.update('nt_user', `nt_user_id=${u.id}`, { + password: hash, + pass_salt: salt, + }), + ) + } catch (err) { + console.warn(`password hash upgrade failed for user ${authTry.username}`, err) + } } for (const f of ['password', 'pass_salt']) { delete u[f] // SECURITY: no longer needed diff --git a/lib/user/store/toml.js b/lib/user/store/toml.js index 8a06ec9..5d2d1c7 100644 --- a/lib/user/store/toml.js +++ b/lib/user/store/toml.js @@ -86,7 +86,26 @@ class UserRepoTOML extends UserBase { if (u.username !== username) continue if (u.deleted) continue - if (await this.validPassword(authTry.password, u.password, authTry.username, u.pass_salt)) { + const { valid, needsUpgrade } = await this.validPassword( + authTry.password, + u.password, + authTry.username, + u.pass_salt, + ) + if (valid) { + // best effort, as in the MySQL repo: a failed rewrite must not cost + // the user their login + if (needsUpgrade) { + try { + const salt = this.generateSalt() + u.pass_salt = salt + u.password = await this.hashForStorage(authTry.password, salt) + await this._save(users) + } catch (err) { + console.warn(`password hash upgrade failed for user ${authTry.username}`, err) + } + } + const result = { ...u } for (const f of ['password', 'pass_salt', 'permissions']) delete result[f] const g = { id: result.gid, name: groupName } diff --git a/lib/user/test/index.js b/lib/user/test/index.js index 2499d2c..9c6d54f 100644 --- a/lib/user/test/index.js +++ b/lib/user/test/index.js @@ -9,6 +9,8 @@ import Mysql from '../../mysql.js' import userJson from './user.json' with { type: 'json' } import groupJson from '../../group/test/group.json' with { type: 'json' } +const storeType = process.env.NICTOOL_DATA_STORE ?? 'mysql' + // This suite soft-deletes and restores its user, so it uses private fixtures // rather than the shared 4096 user/group that the concurrently-run permission // and session suites depend on staying present. @@ -169,7 +171,10 @@ describe('user', function () { }) }) - describe('password upgrade on login', () => { + // these drive the store through raw SQL, so they only apply to the MySQL repo. + // Left running under TOML they hang: the pool never connects and its open + // handle keeps the test process alive forever. + describe('password upgrade on login', { skip: storeType !== 'mysql' }, () => { const upgradeUserId = 4200 const upgradeUser = { nt_user_id: upgradeUserId, @@ -186,10 +191,9 @@ describe('user', function () { } async function getDbPassword() { - const rows = await Mysql.execute( - 'SELECT password, pass_salt FROM nt_user WHERE nt_user_id = ?', - [upgradeUserId], - ) + const rows = await Mysql.execute('SELECT password, pass_salt FROM nt_user WHERE nt_user_id = ?', [ + upgradeUserId, + ]) return rows[0] } @@ -198,15 +202,21 @@ describe('user', function () { async function insertUser(password, passSalt) { await Mysql.execute( 'INSERT INTO nt_user (nt_user_id, nt_group_id, username, email, first_name, last_name, password, pass_salt) VALUES (?, ?, ?, ?, ?, ?, ?, ?)', - [upgradeUserId, upgradeUser.nt_group_id, upgradeUser.username, upgradeUser.email, upgradeUser.first_name, upgradeUser.last_name, password, passSalt], + [ + upgradeUserId, + upgradeUser.nt_group_id, + upgradeUser.username, + upgradeUser.email, + upgradeUser.first_name, + upgradeUser.last_name, + password, + passSalt, + ], ) } async function cleanup() { - await Mysql.execute( - 'DELETE FROM nt_user WHERE nt_user_id = ?', - [upgradeUserId], - ) + await Mysql.execute('DELETE FROM nt_user WHERE nt_user_id = ?', [upgradeUserId]) } before(cleanup) From e5c6b99d1c0497dae0bd2541f52e5e17def7ec2e Mon Sep 17 00:00:00 2001 From: Matt Simerson Date: Sat, 25 Jul 2026 14:26:36 -0700 Subject: [PATCH 3/6] fix: move user mysql tests into user/test/mysql --- lib/user/test/index.js | 130 ----------------------------------------- lib/user/test/mysql.js | 127 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 127 insertions(+), 130 deletions(-) create mode 100644 lib/user/test/mysql.js diff --git a/lib/user/test/index.js b/lib/user/test/index.js index 9c6d54f..105be19 100644 --- a/lib/user/test/index.js +++ b/lib/user/test/index.js @@ -1,16 +1,12 @@ import assert from 'node:assert/strict' -import crypto from 'node:crypto' import { describe, it, after, before } from 'node:test' import User from '../index.js' import Group from '../../group/index.js' -import Mysql from '../../mysql.js' import userJson from './user.json' with { type: 'json' } import groupJson from '../../group/test/group.json' with { type: 'json' } -const storeType = process.env.NICTOOL_DATA_STORE ?? 'mysql' - // This suite soft-deletes and restores its user, so it uses private fixtures // rather than the shared 4096 user/group that the concurrently-run permission // and session suites depend on staying present. @@ -170,130 +166,4 @@ describe('user', function () { assert.ok(u) }) }) - - // these drive the store through raw SQL, so they only apply to the MySQL repo. - // Left running under TOML they hang: the pool never connects and its open - // handle keeps the test process alive forever. - describe('password upgrade on login', { skip: storeType !== 'mysql' }, () => { - const upgradeUserId = 4200 - const upgradeUser = { - nt_user_id: upgradeUserId, - nt_group_id: groupCase.id, - username: 'upgrade-test', - email: 'upgrade-test@example.com', - first_name: 'Upgrade', - last_name: 'Test', - } - const testPass = 'UpgradeMe!123' - const authCreds = { - username: `${upgradeUser.username}@${groupCase.name}`, - password: testPass, - } - - async function getDbPassword() { - const rows = await Mysql.execute('SELECT password, pass_salt FROM nt_user WHERE nt_user_id = ?', [ - upgradeUserId, - ]) - return rows[0] - } - - // Raw SQL so we can insert specific legacy password formats (plain text, - // SHA-1, PBKDF2-5000) that User.create() would hash on the way in. - async function insertUser(password, passSalt) { - await Mysql.execute( - 'INSERT INTO nt_user (nt_user_id, nt_group_id, username, email, first_name, last_name, password, pass_salt) VALUES (?, ?, ?, ?, ?, ?, ?, ?)', - [ - upgradeUserId, - upgradeUser.nt_group_id, - upgradeUser.username, - upgradeUser.email, - upgradeUser.first_name, - upgradeUser.last_name, - password, - passSalt, - ], - ) - } - - async function cleanup() { - await Mysql.execute('DELETE FROM nt_user WHERE nt_user_id = ?', [upgradeUserId]) - } - - before(cleanup) - after(cleanup) - - it('upgrades plain text password to self-describing PBKDF2 on login', async () => { - await cleanup() - await insertUser(testPass, null) - - const result = await User.authenticate(authCreds) - assert.ok(result, 'login should succeed') - - const row = await getDbPassword() - assert.ok(row.pass_salt, 'pass_salt should be set after upgrade') - assert.notEqual(row.password, testPass, 'password should be hashed') - assert.ok(row.password.includes('$'), 'password should be in self-describing format') - - // verify round-trip: can still log in with the upgraded hash - const again = await User.authenticate(authCreds) - assert.ok(again, 'login should succeed after upgrade') - await cleanup() - }) - - it('upgrades SHA1 password to self-describing PBKDF2 on login', async () => { - // authenticate() passes the full authTry.username (including @group) to - // validPassword(), so the HMAC key must match that full string - const sha1Hash = crypto - .createHmac('sha1', authCreds.username.toLowerCase()) - .update(testPass) - .digest('hex') - await cleanup() - await insertUser(sha1Hash, null) - - const result = await User.authenticate(authCreds) - assert.ok(result, 'login should succeed with SHA1 hash') - - const row = await getDbPassword() - assert.ok(row.pass_salt, 'pass_salt should be set after upgrade') - assert.notEqual(row.password, sha1Hash, 'password should be re-hashed') - assert.ok(row.password.includes('$'), 'password should be in self-describing format') - - const again = await User.authenticate(authCreds) - assert.ok(again, 'login should succeed after upgrade') - await cleanup() - }) - - it('upgrades PBKDF2-5000 to self-describing format on login', async () => { - const legacySalt = User.generateSalt() - const legacyHash = await User.hashAuthPbkdf2(testPass, legacySalt, 5000) - await cleanup() - await insertUser(legacyHash, legacySalt) - - const result = await User.authenticate(authCreds) - assert.ok(result, 'login should succeed with legacy PBKDF2') - - const row = await getDbPassword() - assert.notEqual(row.password, legacyHash, 'password should be re-hashed') - assert.notEqual(row.pass_salt, legacySalt, 'salt should be regenerated') - assert.ok(row.password.includes('$'), 'password should be in self-describing format') - - const again = await User.authenticate(authCreds) - assert.ok(again, 'login should succeed after upgrade') - await cleanup() - }) - - it('does not re-hash password already in self-describing format', async () => { - const salt = User.generateSalt() - const hash = await User.hashForStorage(testPass, salt) - await cleanup() - await insertUser(hash, salt) - - await User.authenticate(authCreds) - - const row = await getDbPassword() - assert.equal(row.password, hash, 'password should be unchanged') - assert.equal(row.pass_salt, salt, 'salt should be unchanged') - await cleanup() - }) - }) }) diff --git a/lib/user/test/mysql.js b/lib/user/test/mysql.js new file mode 100644 index 0000000..805b6da --- /dev/null +++ b/lib/user/test/mysql.js @@ -0,0 +1,127 @@ +import assert from 'node:assert/strict' +import crypto from 'node:crypto' +import { describe, it, before, after } from 'node:test' + +import User from '../index.js' +import Group from '../../group/index.js' + +import groupJson from '../../group/test/group.json' with { type: 'json' } + +// Distinct ids so this never races with index.js, which runs concurrently +const groupCase = { ...groupJson, id: 9002, name: 'usertest-mysql.example.com' } +const upgradeUserId = 9002 +const upgradeUser = { + nt_user_id: upgradeUserId, + nt_group_id: groupCase.id, + username: 'upgrade-test', + email: 'upgrade-test@example.com', + first_name: 'Upgrade', + last_name: 'Test', +} +const testPass = 'UpgradeMe!123' +const authCreds = { + username: `${upgradeUser.username}@${groupCase.name}`, + password: testPass, +} + +const SELF_DESCRIBING = /^\d+\$[0-9a-f]{64}$/ + +before(async () => { + await Group.create(groupCase) +}) + +after(async () => { + await User.destroy({ id: upgradeUserId }) + await Group.destroy({ id: groupCase.id }) + await User.disconnect() +}) + +async function storedCredentials() { + const rows = await User.mysql.execute('SELECT password, pass_salt FROM nt_user WHERE nt_user_id = ?', [ + upgradeUserId, + ]) + return rows[0] +} + +// Raw SQL so we can plant the legacy password formats (plain text, SHA-1, +// PBKDF2-5000) that User.create() would hash on the way in. +async function seedUser(password, passSalt) { + await User.destroy({ id: upgradeUserId }) + await User.mysql.execute( + 'INSERT INTO nt_user (nt_user_id, nt_group_id, username, email, first_name, last_name, password, pass_salt) VALUES (?, ?, ?, ?, ?, ?, ?, ?)', + [ + upgradeUserId, + upgradeUser.nt_group_id, + upgradeUser.username, + upgradeUser.email, + upgradeUser.first_name, + upgradeUser.last_name, + password, + passSalt, + ], + ) +} + +describe('user (mysql)', function () { + describe('password upgrade on login', function () { + after(async () => { + await User.destroy({ id: upgradeUserId }) + }) + + it('upgrades plain text password to self-describing PBKDF2 on login', async () => { + await seedUser(testPass, null) + + assert.ok(await User.authenticate(authCreds), 'login should succeed') + + const creds = await storedCredentials() + assert.ok(creds.pass_salt, 'pass_salt should be set after upgrade') + assert.match(creds.password, SELF_DESCRIBING) + + assert.ok(await User.authenticate(authCreds), 'login should succeed after upgrade') + }) + + it('upgrades SHA1 password to self-describing PBKDF2 on login', async () => { + // authenticate() passes the full authTry.username (including @group) to + // validPassword(), so the HMAC key must match that full string + const sha1Hash = crypto + .createHmac('sha1', authCreds.username.toLowerCase()) + .update(testPass) + .digest('hex') + await seedUser(sha1Hash, null) + + assert.ok(await User.authenticate(authCreds), 'login should succeed with SHA1 hash') + + const creds = await storedCredentials() + assert.ok(creds.pass_salt, 'pass_salt should be set after upgrade') + assert.match(creds.password, SELF_DESCRIBING) + + assert.ok(await User.authenticate(authCreds), 'login should succeed after upgrade') + }) + + it('upgrades PBKDF2-5000 to self-describing format on login', async () => { + const legacySalt = User.generateSalt() + const legacyHash = await User.hashAuthPbkdf2(testPass, legacySalt, 5000) + await seedUser(legacyHash, legacySalt) + + assert.ok(await User.authenticate(authCreds), 'login should succeed with legacy PBKDF2') + + const creds = await storedCredentials() + assert.match(creds.password, SELF_DESCRIBING) + assert.notEqual(creds.pass_salt, legacySalt, 'salt should be regenerated') + + assert.ok(await User.authenticate(authCreds), 'login should succeed after upgrade') + }) + + it('does not re-hash password already in self-describing format', async () => { + const salt = User.generateSalt() + const hash = await User.hashForStorage(testPass, salt) + await seedUser(hash, salt) + + await User.authenticate(authCreds) + + const creds = await storedCredentials() + assert.equal(creds.password, hash, 'password should be unchanged') + assert.equal(creds.pass_salt, salt, 'salt should be unchanged') + }) + }) +}) From eec61bb5b0c631324e2a6c4b511b2ea108e1daeb Mon Sep 17 00:00:00 2001 From: Matt Simerson Date: Sat, 25 Jul 2026 14:29:38 -0700 Subject: [PATCH 4/6] address copilot review comments --- lib/user/store/base.js | 34 +++++++++++++++++++++++----------- 1 file changed, 23 insertions(+), 11 deletions(-) diff --git a/lib/user/store/base.js b/lib/user/store/base.js index 7accf91..256eeb3 100644 --- a/lib/user/store/base.js +++ b/lib/user/store/base.js @@ -1,20 +1,32 @@ import crypto from 'node:crypto' +const DEFAULT_ITERATIONS = 220000 +const LEGACY_ITERATIONS = 5000 +const SELF_DESCRIBING_RE = /^(\d+)\$([0-9a-f]{64})$/ + +// PBKDF2-SHA512 costs ~40ms at the 220k default. This ceiling bounds both a +// misconfigured env var and a tampered DB row: a stored count is untrusted +// input to a CPU bound function, so cap the amplification at roughly 4x. +const MAX_ITERATIONS = 1_000_000 + +function validIterations(n) { + return Number.isSafeInteger(n) && n >= 1 && n <= MAX_ITERATIONS +} + const PBKDF2_ITERATIONS = (() => { const raw = process.env.PBKDF2_ITERATIONS - if (raw === undefined) return 220000 - const parsed = parseInt(raw, 10) - if (Number.isNaN(parsed) || parsed < 1) { - console.warn(`PBKDF2_ITERATIONS="${raw}" is not a valid positive integer, using 220000`) - return 220000 + if (raw === undefined) return DEFAULT_ITERATIONS + + // Number() rather than parseInt(), which would read "220000zzz" as 220000 + const parsed = Number(raw) + if (!validIterations(parsed)) { + console.warn( + `PBKDF2_ITERATIONS="${raw}" is not an integer in 1..${MAX_ITERATIONS}, using ${DEFAULT_ITERATIONS}`, + ) + return DEFAULT_ITERATIONS } return parsed })() -const LEGACY_ITERATIONS = 5000 -const SELF_DESCRIBING_RE = /^(\d+)\$([0-9a-f]{64})$/ - -// a stored iteration count is attacker-influenced if the DB is tampered with -const MAX_STORED_ITERATIONS = 10_000_000 function equalsConstantTime(a, b) { const bufA = Buffer.from(a, 'hex') @@ -114,7 +126,7 @@ class UserBase { if (m) { const storedIters = parseInt(m[1], 10) const storedHashHex = m[2] - if (storedIters < 1 || storedIters > MAX_STORED_ITERATIONS) { + if (!validIterations(storedIters)) { console.warn(`refusing stored PBKDF2 iteration count ${storedIters} for user ${username}`) return invalid } From 829ebeb37e2694f09f1cdcf5a66ce4d47081cf6b Mon Sep 17 00:00:00 2001 From: Matt Simerson Date: Sat, 25 Jul 2026 15:10:05 -0700 Subject: [PATCH 5/6] removed user creds processing to user/credentials --- lib/user/credentials.js | 140 ++++++++++++++++++++++++++++++++++++++++ lib/user/store/base.js | 123 +---------------------------------- lib/user/store/mysql.js | 16 ++--- lib/user/store/toml.js | 10 ++- lib/user/test/index.js | 23 +++---- lib/user/test/mysql.js | 9 +-- routes/user.js | 5 +- 7 files changed, 172 insertions(+), 154 deletions(-) create mode 100644 lib/user/credentials.js diff --git a/lib/user/credentials.js b/lib/user/credentials.js new file mode 100644 index 0000000..4207c32 --- /dev/null +++ b/lib/user/credentials.js @@ -0,0 +1,140 @@ +import crypto from 'node:crypto' + +const DEFAULT_ITERATIONS = 220000 +const LEGACY_ITERATIONS = 5000 +const SELF_DESCRIBING_RE = /^(\d+)\$([0-9a-f]{64})$/ + +// PBKDF2-SHA512 costs ~40ms at the 220k default. This ceiling bounds both a +// misconfigured env var and a tampered DB row: a stored count is untrusted +// input to a CPU bound function, so cap the amplification at roughly 4x. +const MAX_ITERATIONS = 1_000_000 + +function validIterations(n) { + return Number.isSafeInteger(n) && n >= 1 && n <= MAX_ITERATIONS +} + +const PBKDF2_ITERATIONS = (() => { + const raw = process.env.PBKDF2_ITERATIONS + if (raw === undefined) return DEFAULT_ITERATIONS + + // Number() rather than parseInt(), which would read "220000zzz" as 220000 + const parsed = Number(raw) + if (!validIterations(parsed)) { + console.warn( + `PBKDF2_ITERATIONS="${raw}" is not an integer in 1..${MAX_ITERATIONS}, using ${DEFAULT_ITERATIONS}`, + ) + return DEFAULT_ITERATIONS + } + return parsed +})() + +function equalsConstantTime(a, b) { + const bufA = Buffer.from(a, 'hex') + const bufB = Buffer.from(b, 'hex') + if (bufA.length !== bufB.length) return false + return crypto.timingSafeEqual(bufA, bufB) +} + +/** + * Password hashing and verification, independent of how users are stored. + * + * Hashes are stored self-describing as `iterations$hexHash`, so verification + * never has to guess a format and a wrong password costs exactly one PBKDF2 + * computation no matter how many legacy formats have existed. + */ +class Credentials { + constructor(args = {}) { + this.debug = args?.debug ?? false + } + + generateSalt(length = 16) { + const chars = Array.from({ length: 87 }, (_, i) => String.fromCharCode(i + 40)) // ASCII 40–126 + let salt = '' + for (let i = 0; i < length; i++) { + salt += chars[Math.floor(Math.random() * 87)] + } + return salt + } + + async hashAuthPbkdf2(pass, salt, iterations = PBKDF2_ITERATIONS) { + return new Promise((resolve, reject) => { + crypto.pbkdf2(pass, salt, iterations, 32, 'sha512', (err, derivedKey) => { + if (err) return reject(err) + resolve(derivedKey.toString('hex')) + }) + }) + } + + async hashForStorage(pass, salt, iterations = PBKDF2_ITERATIONS) { + const hex = await this.hashAuthPbkdf2(pass, salt, iterations) + return `${iterations}$${hex}` + } + + /** + * The password/pass_salt pair to persist for a plain text password. + * + * Omit `salt` to get a fresh one, which is what a password change and a + * hash upgrade both want. Pass an existing salt only to preserve a caller + * supplied one, as create() does for fixtures and imports. + */ + async forStorage(pass, salt = this.generateSalt()) { + return { password: await this.hashForStorage(pass, salt), pass_salt: salt } + } + + async validPassword(passTry, passDb, username, salt) { + const invalid = { valid: false, needsUpgrade: false } + + if (!salt && passTry === passDb) { + return { valid: true, needsUpgrade: true } + } + + if (salt) { + // Self-describing format: "iterations$hexHash" — single hash, no fallback + const m = SELF_DESCRIBING_RE.exec(passDb) + if (m) { + const storedIters = parseInt(m[1], 10) + const storedHashHex = m[2] + if (!validIterations(storedIters)) { + console.warn(`refusing stored PBKDF2 iteration count ${storedIters} for user ${username}`) + return invalid + } + + let hashed + try { + hashed = await this.hashAuthPbkdf2(passTry, salt, storedIters) + } catch (err) { + console.error(`PBKDF2 failed for user ${username}`, err) + return invalid + } + + if (this.debug) console.log(`self-describing: ${hashed}`) + if (equalsConstantTime(hashed, storedHashHex)) { + return { valid: true, needsUpgrade: storedIters < PBKDF2_ITERATIONS } + } + return invalid + } + + // Raw hex (legacy NicTool 2 format, implicitly 5000 iterations) + const legacy = await this.hashAuthPbkdf2(passTry, salt, LEGACY_ITERATIONS) + if (this.debug) console.log(`legacy: ${legacy}`) + if (/^[0-9a-f]{64}$/.test(passDb) && equalsConstantTime(legacy, passDb)) { + return { valid: true, needsUpgrade: true } + } + return invalid + } + + // Check for HMAC SHA-1 password + if (/^[0-9a-f]{40}$/.test(passDb)) { + const digest = crypto.createHmac('sha1', username.toLowerCase()).update(passTry).digest('hex') + if (this.debug) console.log(`digest: ${digest}`) + if (equalsConstantTime(digest, passDb)) { + return { valid: true, needsUpgrade: true } + } + } + + return invalid + } +} + +export default new Credentials() +export { Credentials } diff --git a/lib/user/store/base.js b/lib/user/store/base.js index 256eeb3..aacf398 100644 --- a/lib/user/store/base.js +++ b/lib/user/store/base.js @@ -1,45 +1,9 @@ -import crypto from 'node:crypto' - -const DEFAULT_ITERATIONS = 220000 -const LEGACY_ITERATIONS = 5000 -const SELF_DESCRIBING_RE = /^(\d+)\$([0-9a-f]{64})$/ - -// PBKDF2-SHA512 costs ~40ms at the 220k default. This ceiling bounds both a -// misconfigured env var and a tampered DB row: a stored count is untrusted -// input to a CPU bound function, so cap the amplification at roughly 4x. -const MAX_ITERATIONS = 1_000_000 - -function validIterations(n) { - return Number.isSafeInteger(n) && n >= 1 && n <= MAX_ITERATIONS -} - -const PBKDF2_ITERATIONS = (() => { - const raw = process.env.PBKDF2_ITERATIONS - if (raw === undefined) return DEFAULT_ITERATIONS - - // Number() rather than parseInt(), which would read "220000zzz" as 220000 - const parsed = Number(raw) - if (!validIterations(parsed)) { - console.warn( - `PBKDF2_ITERATIONS="${raw}" is not an integer in 1..${MAX_ITERATIONS}, using ${DEFAULT_ITERATIONS}`, - ) - return DEFAULT_ITERATIONS - } - return parsed -})() - -function equalsConstantTime(a, b) { - const bufA = Buffer.from(a, 'hex') - const bufB = Buffer.from(b, 'hex') - if (bufA.length !== bufB.length) return false - return crypto.timingSafeEqual(bufA, bufB) -} - /** - * User domain class – pure attributes and password business logic. + * Base user repository – the persistence contract, nothing more. * * Has zero knowledge of how users are persisted. All user repository classes - * must extend this class and implement the repo contract methods. + * must extend this class and implement the repo contract methods. Password + * hashing and verification live in ../credentials.js. * * Repo contract: * authenticate(authTry) → { user, group } | undefined @@ -85,87 +49,6 @@ class UserBase { async destroy(_args) { throw new Error('destroy() not implemented by this repo') } - - // ------------------------------------------------------------------------- - // Password business logic - // ------------------------------------------------------------------------- - - generateSalt(length = 16) { - const chars = Array.from({ length: 87 }, (_, i) => String.fromCharCode(i + 40)) // ASCII 40–126 - let salt = '' - for (let i = 0; i < length; i++) { - salt += chars[Math.floor(Math.random() * 87)] - } - return salt - } - - async hashAuthPbkdf2(pass, salt, iterations = PBKDF2_ITERATIONS) { - return new Promise((resolve, reject) => { - crypto.pbkdf2(pass, salt, iterations, 32, 'sha512', (err, derivedKey) => { - if (err) return reject(err) - resolve(derivedKey.toString('hex')) - }) - }) - } - - async hashForStorage(pass, salt, iterations = PBKDF2_ITERATIONS) { - const hex = await this.hashAuthPbkdf2(pass, salt, iterations) - return `${iterations}$${hex}` - } - - async validPassword(passTry, passDb, username, salt) { - const invalid = { valid: false, needsUpgrade: false } - - if (!salt && passTry === passDb) { - return { valid: true, needsUpgrade: true } - } - - if (salt) { - // Self-describing format: "iterations$hexHash" — single hash, no fallback - const m = SELF_DESCRIBING_RE.exec(passDb) - if (m) { - const storedIters = parseInt(m[1], 10) - const storedHashHex = m[2] - if (!validIterations(storedIters)) { - console.warn(`refusing stored PBKDF2 iteration count ${storedIters} for user ${username}`) - return invalid - } - - let hashed - try { - hashed = await this.hashAuthPbkdf2(passTry, salt, storedIters) - } catch (err) { - console.error(`PBKDF2 failed for user ${username}`, err) - return invalid - } - - if (this.debug) console.log(`self-describing: ${hashed}`) - if (equalsConstantTime(hashed, storedHashHex)) { - return { valid: true, needsUpgrade: storedIters < PBKDF2_ITERATIONS } - } - return invalid - } - - // Raw hex (legacy NicTool 2 format, implicitly 5000 iterations) - const legacy = await this.hashAuthPbkdf2(passTry, salt, LEGACY_ITERATIONS) - if (this.debug) console.log(`legacy: ${legacy}`) - if (/^[0-9a-f]{64}$/.test(passDb) && equalsConstantTime(legacy, passDb)) { - return { valid: true, needsUpgrade: true } - } - return invalid - } - - // Check for HMAC SHA-1 password - if (/^[0-9a-f]{40}$/.test(passDb)) { - const digest = crypto.createHmac('sha1', username.toLowerCase()).update(passTry).digest('hex') - if (this.debug) console.log(`digest: ${digest}`) - if (equalsConstantTime(digest, passDb)) { - return { valid: true, needsUpgrade: true } - } - } - - return invalid - } } export default UserBase diff --git a/lib/user/store/mysql.js b/lib/user/store/mysql.js index 53d1a49..85b55e6 100644 --- a/lib/user/store/mysql.js +++ b/lib/user/store/mysql.js @@ -1,5 +1,6 @@ import Mysql from '../../mysql.js' import Config from '../../config.js' +import Credentials from '../credentials.js' import UserBase from './base.js' import Permission from '../../permission/index.js' import { mapToDbColumn } from '../../util.js' @@ -36,7 +37,7 @@ class UserRepoMySQL extends UserBase { AND g.name = ?` for (const u of await Mysql.execute(query, [username, groupName])) { - const { valid, needsUpgrade } = await this.validPassword( + const { valid, needsUpgrade } = await Credentials.validPassword( authTry.password, u.password, authTry.username, @@ -47,14 +48,8 @@ class UserRepoMySQL extends UserBase { // they simply get upgraded on a later attempt if (needsUpgrade) { try { - const salt = this.generateSalt() - const hash = await this.hashForStorage(authTry.password, salt) - await Mysql.execute( - ...Mysql.update('nt_user', `nt_user_id=${u.id}`, { - password: hash, - pass_salt: salt, - }), - ) + const creds = await Credentials.forStorage(authTry.password) + await Mysql.execute(...Mysql.update('nt_user', `nt_user_id=${u.id}`, creds)) } catch (err) { console.warn(`password hash upgrade failed for user ${authTry.username}`, err) } @@ -86,8 +81,7 @@ class UserRepoMySQL extends UserBase { delete args.inherit_group_permissions if (args.password) { - if (!args.pass_salt) args.pass_salt = this.generateSalt() - args.password = await this.hashForStorage(args.password, args.pass_salt) + Object.assign(args, await Credentials.forStorage(args.password, args.pass_salt)) } const userId = await Mysql.execute(...Mysql.insert(`nt_user`, mapToDbColumn(args, userDbMap))) diff --git a/lib/user/store/toml.js b/lib/user/store/toml.js index 5d2d1c7..3ec4715 100644 --- a/lib/user/store/toml.js +++ b/lib/user/store/toml.js @@ -5,6 +5,7 @@ import { fileURLToPath } from 'node:url' import { parse, stringify } from 'smol-toml' import Config from '../../config.js' +import Credentials from '../credentials.js' import UserBase from './base.js' const __dirname = path.dirname(fileURLToPath(import.meta.url)) @@ -86,7 +87,7 @@ class UserRepoTOML extends UserBase { if (u.username !== username) continue if (u.deleted) continue - const { valid, needsUpgrade } = await this.validPassword( + const { valid, needsUpgrade } = await Credentials.validPassword( authTry.password, u.password, authTry.username, @@ -97,9 +98,7 @@ class UserRepoTOML extends UserBase { // the user their login if (needsUpgrade) { try { - const salt = this.generateSalt() - u.pass_salt = salt - u.password = await this.hashForStorage(authTry.password, salt) + Object.assign(u, await Credentials.forStorage(authTry.password)) await this._save(users) } catch (err) { console.warn(`password hash upgrade failed for user ${authTry.username}`, err) @@ -170,8 +169,7 @@ class UserRepoTOML extends UserBase { delete args.inherit_group_permissions if (args.password) { - if (!args.pass_salt) args.pass_salt = this.generateSalt() - args.password = await this.hashForStorage(args.password, args.pass_salt) + Object.assign(args, await Credentials.forStorage(args.password, args.pass_salt)) } if (inherit === false) { diff --git a/lib/user/test/index.js b/lib/user/test/index.js index 105be19..4a25f52 100644 --- a/lib/user/test/index.js +++ b/lib/user/test/index.js @@ -3,6 +3,7 @@ import { describe, it, after, before } from 'node:test' import User from '../index.js' import Group from '../../group/index.js' +import Credentials from '../credentials.js' import userJson from './user.json' with { type: 'json' } import groupJson from '../../group/test/group.json' with { type: 'json' } @@ -90,40 +91,40 @@ describe('user', function () { describe('validPassword', function () { it('auths user with plain text password', async () => { - const r = await User.validPassword('test', 'test', 'demo', '') + const r = await Credentials.validPassword('test', 'test', 'demo', '') assert.deepEqual(r, { valid: true, needsUpgrade: true }) }) it('auths valid self-describing PBKDF2 password', async () => { const salt = '(ICzAm2.QfCa6.MN' - const hash = await User.hashForStorage('YouGuessedIt!', salt) - const r = await User.validPassword('YouGuessedIt!', hash, 'unit-test', salt) + const hash = await Credentials.hashForStorage('YouGuessedIt!', salt) + const r = await Credentials.validPassword('YouGuessedIt!', hash, 'unit-test', salt) assert.deepEqual(r, { valid: true, needsUpgrade: false }) }) it('rejects invalid self-describing PBKDF2 password', async () => { const salt = '(ICzAm2.QfCa6.MN' - const hash = await User.hashForStorage('YouGuessedIt!', salt) - const r = await User.validPassword('YouMissedIt!', hash, 'unit-test', salt) + const hash = await Credentials.hashForStorage('YouGuessedIt!', salt) + const r = await Credentials.validPassword('YouMissedIt!', hash, 'unit-test', salt) assert.deepEqual(r, { valid: false, needsUpgrade: false }) }) it('auths valid legacy PBKDF2-5000 password', async () => { const salt = '(ICzAm2.QfCa6.MN' - const hash = await User.hashAuthPbkdf2('YouGuessedIt!', salt, 5000) - const r = await User.validPassword('YouGuessedIt!', hash, 'unit-test', salt) + const hash = await Credentials.hashAuthPbkdf2('YouGuessedIt!', salt, 5000) + const r = await Credentials.validPassword('YouGuessedIt!', hash, 'unit-test', salt) assert.deepEqual(r, { valid: true, needsUpgrade: true }) }) it('rejects invalid legacy PBKDF2-5000 password', async () => { const salt = '(ICzAm2.QfCa6.MN' - const hash = await User.hashAuthPbkdf2('YouGuessedIt!', salt, 5000) - const r = await User.validPassword('YouMissedIt!', hash, 'unit-test', salt) + const hash = await Credentials.hashAuthPbkdf2('YouGuessedIt!', salt, 5000) + const r = await Credentials.validPassword('YouMissedIt!', hash, 'unit-test', salt) assert.deepEqual(r, { valid: false, needsUpgrade: false }) }) it('auths valid SHA1 password', async () => { - const r = await User.validPassword( + const r = await Credentials.validPassword( 'OhNoYouDont', '083007777a5241d01abba70c938c60d80be60027', 'unit-test', @@ -132,7 +133,7 @@ describe('user', function () { }) it('rejects invalid SHA1 password', async () => { - const r = await User.validPassword( + const r = await Credentials.validPassword( 'OhNoYouDont', '083007777a5241d01abba7Oc938c60d80be60027', 'unit-test', diff --git a/lib/user/test/mysql.js b/lib/user/test/mysql.js index 805b6da..a52cc75 100644 --- a/lib/user/test/mysql.js +++ b/lib/user/test/mysql.js @@ -4,6 +4,7 @@ import { describe, it, before, after } from 'node:test' import User from '../index.js' import Group from '../../group/index.js' +import Credentials from '../credentials.js' import groupJson from '../../group/test/group.json' with { type: 'json' } @@ -99,8 +100,8 @@ describe('user (mysql)', function () { }) it('upgrades PBKDF2-5000 to self-describing format on login', async () => { - const legacySalt = User.generateSalt() - const legacyHash = await User.hashAuthPbkdf2(testPass, legacySalt, 5000) + const legacySalt = Credentials.generateSalt() + const legacyHash = await Credentials.hashAuthPbkdf2(testPass, legacySalt, 5000) await seedUser(legacyHash, legacySalt) assert.ok(await User.authenticate(authCreds), 'login should succeed with legacy PBKDF2') @@ -113,8 +114,8 @@ describe('user (mysql)', function () { }) it('does not re-hash password already in self-describing format', async () => { - const salt = User.generateSalt() - const hash = await User.hashForStorage(testPass, salt) + const salt = Credentials.generateSalt() + const hash = await Credentials.hashForStorage(testPass, salt) await seedUser(hash, salt) await User.authenticate(authCreds) diff --git a/routes/user.js b/routes/user.js index 5216c43..eeef463 100644 --- a/routes/user.js +++ b/routes/user.js @@ -1,6 +1,7 @@ import validate from '@nictool/validate' import User from '../lib/user/index.js' +import Credentials from '../lib/user/credentials.js' import { meta } from '../lib/util.js' function UserRoutes(server) { @@ -134,9 +135,9 @@ function UserRoutes(server) { const id = parseInt(request.params.id, 10) const args = { ...request.payload, id } + // no salt passed: a password change always gets a fresh one if (args.password) { - args.pass_salt = User.generateSalt() - args.password = await User.hashForStorage(args.password, args.pass_salt) + Object.assign(args, await Credentials.forStorage(args.password)) } await User.put(args) From b64f02305da6ba7b8764e26c7ca68f86261cc5fe Mon Sep 17 00:00:00 2001 From: Matt Simerson Date: Sat, 25 Jul 2026 15:17:36 -0700 Subject: [PATCH 6/6] only compute PBKDF2 hash when needed --- lib/user/credentials.js | 16 ++++++++++++---- lib/user/store/base.js | 2 +- 2 files changed, 13 insertions(+), 5 deletions(-) diff --git a/lib/user/credentials.js b/lib/user/credentials.js index 4207c32..0c570f6 100644 --- a/lib/user/credentials.js +++ b/lib/user/credentials.js @@ -3,6 +3,8 @@ import crypto from 'node:crypto' const DEFAULT_ITERATIONS = 220000 const LEGACY_ITERATIONS = 5000 const SELF_DESCRIBING_RE = /^(\d+)\$([0-9a-f]{64})$/ +const LEGACY_PBKDF2_RE = /^[0-9a-f]{64}$/ +const LEGACY_SHA1_RE = /^[0-9a-f]{40}$/ // PBKDF2-SHA512 costs ~40ms at the 220k default. This ceiling bounds both a // misconfigured env var and a tampered DB row: a stored count is untrusted @@ -114,17 +116,23 @@ class Credentials { return invalid } - // Raw hex (legacy NicTool 2 format, implicitly 5000 iterations) + // Raw hex (legacy NicTool 2 format, implicitly 5000 iterations). Reject + // on shape before hashing, so an unrecognized stored value still costs + // an attempt exactly zero PBKDF2 computations. + if (!LEGACY_PBKDF2_RE.test(passDb)) return invalid + const legacy = await this.hashAuthPbkdf2(passTry, salt, LEGACY_ITERATIONS) if (this.debug) console.log(`legacy: ${legacy}`) - if (/^[0-9a-f]{64}$/.test(passDb) && equalsConstantTime(legacy, passDb)) { + if (equalsConstantTime(legacy, passDb)) { return { valid: true, needsUpgrade: true } } return invalid } - // Check for HMAC SHA-1 password - if (/^[0-9a-f]{40}$/.test(passDb)) { + // HMAC SHA-1 (NicTool 2). Verified, never written: a successful match + // returns needsUpgrade so the caller rewrites it as PBKDF2 immediately. + // CodeQL js/insufficient-password-hash flags this; dismissed as won't fix. + if (LEGACY_SHA1_RE.test(passDb)) { const digest = crypto.createHmac('sha1', username.toLowerCase()).update(passTry).digest('hex') if (this.debug) console.log(`digest: ${digest}`) if (equalsConstantTime(digest, passDb)) { diff --git a/lib/user/store/base.js b/lib/user/store/base.js index aacf398..2190215 100644 --- a/lib/user/store/base.js +++ b/lib/user/store/base.js @@ -1,5 +1,5 @@ /** - * Base user repository – the persistence contract, nothing more. + * Base user repository – the persistence contract * * Has zero knowledge of how users are persisted. All user repository classes * must extend this class and implement the repo contract methods. Password