feat: self-describing password hashes (iterations$hex)

[aberoham]

Depends on #40 -- will rebase onto main once that merges.

Matt -- this one tackles the password storage situation. Right now the DB has a mix of plain text, SHA-1, and PBKDF2-5000 passwords with no way to tell which is which. The approach here is what I'd call "self-describing" hashes -- the stored format becomes iterations$hexHash, so validPassword() always knows exactly how to verify without guessing.

The thing that bugged me about the naive approach (try current iterations, fall back to 5000, fall back to SHA-1...) is that every failed login pays the cost of all those tiers. That's basically a free DoS amplifier. With self-describing hashes, a wrong password costs exactly one PBKDF2 computation no matter how many legacy formats have existed.

The upgrade path is transparent -- when someone logs in successfully with any legacy format, their hash gets silently rewritten to 220000$hex with a fresh salt. Future iteration bumps just need a PBKDF2_ITERATIONS env var change, zero code touches, and users upgrade on next login.

The interesting bits to look at are in userBase.js -- hashForStorage() wraps the existing hashAuthPbkdf2() to produce the self-describing format, and validPassword() now returns { valid, needsUpgrade } instead of a bare boolean (cleaner than the mutable state approach claude tried first). All the storage sites (create, authenticate, PUT /user) go through hashForStorage() now.

No new dependencies, just node:crypto.

All 345 tests pass in docker. The upgrade tests cover all four paths -- plain text, SHA-1, PBKDF2-5000, and already-current (should not re-hash). Each one does a round-trip to verify login still works after the upgrade.

Let me know if the format choice makes sense or if you'd rather store it differently.

Abe

[msimerson]

hmmm, needs to be rebased. I like the idea and description.

[msimerson]

Sorry, my PR collided with this. Self-merge once you've rebased.

[msimerson]

Also, in the PR description you should be saying 'fixes #30', right?

[Copilot]

Pull request overview

This PR updates NicTool’s user password storage and verification to use a self-describing PBKDF2 format (<iterations>$<hexHash>), enabling deterministic verification and transparent upgrades of legacy password formats on successful login.

Changes:

• Introduces hashForStorage() and updates PBKDF2 hashing to support configurable iterations (PBKDF2_ITERATIONS, default 220000).
• Updates MySQL auth flow to upgrade legacy passwords (plaintext/SHA-1/PBKDF2-5000) to the self-describing format after successful login.
• Updates hashing at password write sites and extends unit tests to cover verification + upgrade paths.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
routes/user.js	Uses hashForStorage() when updating a user password via PUT /user/{id}.
lib/user/test.js	Updates password-verification expectations and adds end-to-end “upgrade on login” tests using the MySQL store.
lib/user/store/toml.js	Switches TOML user creation to store self-describing hashes.
lib/user/store/mysql.js	Implements login-time password upgrade and stores self-describing hashes for new/updated passwords.
lib/user/store/base.js	Adds iteration configuration, self-describing hash generation, and {valid, needsUpgrade} return shape for validPassword().

[Copilot]

storedIters is parsed from the DB and then passed directly into crypto.pbkdf2(). If the stored password is malformed (e.g. 0$...) or contains an extremely large iteration count, pbkdf2 can throw (500 on login) or become an accidental CPU DoS. Validate storedIters as a safe positive integer within a reasonable max bound and handle pbkdf2 errors by returning { valid: false, needsUpgrade: false } instead of throwing.

[Copilot]

Hash comparisons here use === on hex strings. For password verification it’s safer to use constant-time comparison (e.g. crypto.timingSafeEqual on same-length buffers) to reduce timing side-channels, especially since these are fixed-length derived keys.

[Copilot]

If the upgrade UPDATE fails (DB issue, permissions, transient error), authenticate() will currently throw and block an otherwise-valid login. Consider making the upgrade best-effort (log/report the failure but still return the authenticated user) so availability of logins doesn’t depend on the upgrade write succeeding.

Suggested change

	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)
	}

[Copilot]

authenticate() still treats validPassword() as a boolean, but validPassword() now returns an object ({ valid, needsUpgrade }). Since any object is truthy, TOML auth will accept any password for a matching user. Update this repo to destructure/check .valid (and ideally handle needsUpgrade by re-saving an upgraded hash/salt).

[msimerson]

address issues raised by @copilot