rabbitmq-server icon indicating copy to clipboard operation
rabbitmq-server copied to clipboard

WIP A PBKDF2 implementation for internal database password hashing

Open iinuwa opened this issue 1 year ago • 3 comments

Proposed Changes

This implements PBKDF2 password hash storage for internal authentication. The only built-in hash algorithms available for storing passwords are SHA256 and SHA512, which, while salted with a 32-bit random salt, are not secure for password storage. If an attacker gains access to the hashes stored in RabbitMQ, it would be fairly trivial to crack the salted passwords stored in RabbitMQ.

This change implements PBKDF2 HMAC-SHA512 using crypto:pbkdf2_hmac/5, using the OWASP-recommended iteration count of 210,000. (This OTP method is available since Erlang 24, so it should be available for all currently supported and future versions of RabbitMQ.) The scheme is recorded as _v1 so that future increases to the iteration count can be increased without breaking changes or having to store extra data for the user.

This is more secure against stolen hashes, though decreases performance for evaluating passwords. This means that new AMQP connections, API calls and Management UI sessions will all be more expensive. If merged, the documentation should clearly state this performance impact of using this algorithm, perhaps recommending not to use this algorithm for users that access the Management UI. Since this does not change the default password hash, and will require importing user definitions to apply to a RabbitMQ instance, this can be considered advanced usage.

Fixes #3350.

Types of Changes

What types of changes does your code introduce to this project? Put an x in the boxes that apply

  • [ ] Bug fix (non-breaking change which fixes issue #NNNN)
  • [x] New feature (non-breaking change which adds functionality)
  • [ ] Breaking change (fix or feature that would cause an observable behavior change in existing systems)
  • [ ] Documentation improvements (corrections, new content, etc)
  • [ ] Cosmetic change (whitespace, formatting, etc)
  • [ ] Build system and/or CI

Checklist

Put an x in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask on the mailing list. We're here to help! This is simply a reminder of what we are going to look for before merging your code.

  • [x] I have read the CONTRIBUTING.md document
  • [x] I have signed the CA (see https://cla.pivotal.io/sign/rabbitmq)
  • [ ] I have added tests that prove my fix is effective or that my feature works
  • [ ] All tests pass locally with my changes
  • [ ] If relevant, I have added necessary documentation to https://github.com/rabbitmq/rabbitmq-website
  • [ ] If relevant, I have added this change to the first version(s) in release-notes that I expect to introduce it

Further Comments

  • This could also be implemented as a plugin instead of being added to the core.
  • I have not added tests, as I'm not very familiar with Erlang, Bazel or this codebase, but I have tested this locally.

iinuwa avatar Sep 06 '24 04:09 iinuwa