Skip to content

fix(auth): store passwords with bcrypt and migrate legacy MD5 digests #1432 - #1445

Open
borealisalastor-byte wants to merge 9 commits into
dataease:mainfrom
borealisalastor-byte:fix/1432-bcrypt-password-hash
Open

borealisalastor-byte wants to merge 9 commits into
dataease:mainfrom
borealisalastor-byte:fix/1432-bcrypt-password-hash

Conversation

@borealisalastor-byte

Copy link
Copy Markdown
Contributor

Cause

sys_user.password was stored as an unsalted MD5 digest (common.core.security.md5pwd). As reported in #1432, one dump of the table allows offline cracking of every account, and equal passwords are equal in the table. The digest was also used at every write site: user creation defaults, admin reset, self-service change and the login flow.

Fix

  • storage now goes through bcrypt (get_password_hash) everywhere: the model default, admin reset, login password change, profile password change;
  • verify_password keeps accepting a legacy unsalted MD5 digest, so an existing deployment can still log in, and authenticate() rewrites a verified digest with bcrypt through the request-scoped session, so each row upgrades itself on the next successful login;
  • md5pwd / verify_md5pwd / default_md5_pwd remain only as legacy helpers used by that compatibility path and are documented as such; no production module writes an MD5 password any more.

Tests

  • new guard backend/tests/test_password_hash_migration.py: bcrypt storage (never a 32-hex digest), verification of bcrypt and legacy digests, the login upgrade path (rewrite once on success, never on a failed or non-legacy login) and a repository scan that fails if any module under backend/apps or backend/common hashes a password with the legacy helpers again;
  • backend/tests/test_initial_password_policy.py and backend/tests/test_mcp_parameters_integration.py follow the new helpers instead of the retired MD5 ones.

Verification

  • python -m pytest -q tests/test_password_hash_migration.py tests/test_initial_password_policy.py -> 28 passed, run in backend/ on a checkout of main at a0b6145 with passlib 1.7.4 and bcrypt 4.0.1;
  • on the pre-change tree the new guard cannot even import default_pwd_hash / is_legacy_md5_pwd, and the old authenticate compared verify_md5pwd; the old storage helpers are gone from every call site after this change;
  • the full uv run pytest -q suite was not run here (the environment has no access to the private xpack index), so untouched suites are not covered by this PR;
  • the GitHub web editor added the missing trailing newline to api/login.py (ruff W292 on the current file), which the repository's end-of-file-fixer hook applies as well.

Fixes #1432

…se#1432

Storage moves to bcrypt in the shared helpers; legacy unsalted MD5 digests stay verifiable only for rows created before the migration and are rewritten on the next successful login.
authenticate() verifies through the shared helper and rewrites a verified legacy digest with bcrypt through the request-scoped session.
The login flow verifies through the shared helper and the change-pwd endpoint stores a bcrypt hash.
The model default factory now returns a bcrypt hash of the configured default password.
Resetting a user writes the bcrypt default hash and the self-service update stores bcrypt instead of MD5.
The guard keeps its assertions but stubs get_password_hash and verify_password instead of the retired MD5 helpers.
The MCP integration fixture creates its user through get_password_hash instead of md5pwd.
Covers bcrypt storage, legacy digest compatibility, the login upgrade path and the repository-wide ban on MD5 password writes.
The security guide states that only bcrypt stores passwords and what the legacy MD5 helpers are still allowed to do.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Passwords stored as unsalted MD5 hashes

1 participant