Repository navigation
Support PBKDF2 for password hashing - #17316
Conversation
6cec69f to
a3cb6c7
Compare
|
Could make commit messages shorter. |
a3cb6c7 to
6d985dd
Compare
|
Made the commit messages shorter. Tests were green before that so I expect them to be green again. Ready for review. |
|
Tick the box to add this pull request to the merge queue (same as
|
There was a problem hiding this comment.
🟡 Changes recommended
The PBKDF2 implementation uses an undersized salt and incorrect work factor, while CLI and test integration remain incomplete.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds PBKDF2-HMAC-SHA256 password hashing and prevents management user endpoints from exposing password hashes.
Changes:
- Adds PBKDF2 hashing, tests, and configuration documentation
- Replaces exposed hashes with
has_password - Updates management UI and definition tests
File summaries
| File | Description |
|---|---|
deps/rabbitmq_management/test/rabbit_mgmt_http_SUITE.erl |
Updates user API tests |
deps/rabbitmq_management/priv/www/js/tmpl/users.ejs |
Uses has_password in user listings |
deps/rabbitmq_management/priv/www/js/tmpl/user.ejs |
Uses has_password in user details |
deps/rabbitmq_management/priv/www/api/index.html |
Documents PBKDF2 support |
deps/rabbitmq_management_agent/src/rabbit_mgmt_format.erl |
Hides hashes and exposes password presence |
deps/rabbit/priv/schema/rabbit.schema |
Documents the configuration option |
deps/rabbit/docs/rabbitmq.conf.example |
Adds PBKDF2 configuration guidance |
deps/rabbit_common/test/unit_password_hashing_SUITE.erl |
Tests PBKDF2 hashing |
deps/rabbit_common/src/rabbit_password_hashing_pbkdf2_sha256.erl |
Implements PBKDF2-HMAC-SHA256 |
Review details
- Files reviewed: 9/9 changed files
- Comments generated: 4
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| <a href="https://rabbitmq.com/passwords.html#computing-password-hash">here</a>. | ||
| You may also specify the hash function being used by adding the <code>hashing_algorithm</code> | ||
| key to the body. Currently recognised algorithms are <code>rabbit_password_hashing_sha256</code>, | ||
| key to the body. Currently recognised algorithms are <code>rabbit_password_hashing_sha256</code> |
There was a problem hiding this comment.
Changing this file is OK but that reference has moved to a separate doc guide.
|
@lhoguin thank you for picking this long standing issue up, Loïc. I have pushed a couple of changes that do not need a design or consensus:
Next I'll investigate whether we can make the salt field wider without breaking backwards compatibility. |
|
Also, this PR will break We can consider this acceptable and proactively make that key optional in the Rust HTTP API client and (in turn) |
|
@lhoguin figured a way to make the salt width flexible without breaking compatibility. Storing the module responsible for hashing in the user record was a very wise decision from 2015 or so. We directly benefit from it here. Let me know if you'd like a different implementation, I don't feel strongly about it but this solution is straightforward and backwards-compatible. |
|
Another change from me: fixed a bug that would have manifested itself with wider salts (in the future) but would be enough to be considered a vulnerability. A bug from the future avoided, how about that. |
|
@lhoguin should we include the number of iterations into the module so that we can safely change it down the road, or do you think that it's so unlikely to change in the future that we should not bother? I have added a comment for now to make the risk of |
|
About removing hash_password from API: it's not directly related to the new password algorithm so could be dropped. It should be returned for the administrator tag only, so not a huge concern, and the information is also in definitions. It's more of a defense in depth measure because GET /api/users is called when navigating the UI. If we can not send hash_password for GET /api/users, great, if not, we can drop these commits. I would not send if not required to. The salt width changes look good, thanks! We cannot change the number of iterations once passwords have been hashed. If at some point the recommendations become to double or triple the iterations, we can introduce a new module. But we are likely to have better hashing algorithm when that happens. Right now there's two better algorithms that would be good to have but that we don't have available in OTP (without NIFs): Argon2id and scrypt. But those algorithms are not FIPS approved while the one in this PR is. So we'll see. In any case this isn't going to change too often so no real need to parameterize. I will increase the number of iterations to 600K as that's the recommended number today and there's less risk of issues since this module isn't the default. |
44ae61a to
97f459e
Compare
Add rabbit_password_hashing_pbkdf2_sha256, using 210,000 iterations (OWASP's 2023 minimum), selectable via password_hashing_module. The default is unchanged because this new module has a high cost that may not fit deployments with high churn or many connections.
Replace it with a has_password boolean, which is all the management UI needs.
while at it, remove a factually incorrect comment that arguably has no value and should be simply dropped.
and module-controlled. This is a backwards-compatible way to make room for salts of, say, 16 bytes for the newer hashing algos.
This is a painful couple of changes to explain, so bear with me. There is one bug fixed and one property preserved. 1. Avoid hardcoding a parameter that makes salts longer than 36 bytes slip through a case statement, skipping the hashing altogether. This is future-proofing but if we get it wrong today, we will pay a major price in the future with more hashing modules with even longer salts 2. To make the failing case take roughly as long as succeeding one to prevent username enumeration via a timing side channel (measuring how long a login attempt took to figure out the non-existent accounts) These changes address both by 1. Using an array of zeroes of the salt length returned by the configured module 2. Use an empty value so that hash comparison for a dummy user always fails and causes the slowdown, so a login for a user that does not exist costs as much as one for an existing user with a wrong password, which is what makes enumeration harder, if at all feasible
97f459e to
05c9ca2
Compare
|
Using 600K and treating it as a constant sounds good. As for the password hash in the API response, if we make it optional depending on the tag, we still have to update |
michaelklishin
added
the
do-not-backport
michaelklishin
changed the title
|
First step towards forward compatibility of |
|
And the |
Two things in this PR, a new hashing algorithm, and a management UI improvement. See individual commits.