Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WIP A PBKDF2 implementation for internal database password hashing #12232

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

iinuwa
Copy link

@iinuwa iinuwa commented Sep 6, 2024

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)
  • 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.

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.

@pivotal-cla
Copy link

@iinuwa Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

@mergify mergify bot added the bazel label Sep 6, 2024
@pivotal-cla
Copy link

@iinuwa Thank you for signing the Contributor License Agreement!

@lhoguin
Copy link
Contributor

lhoguin commented Sep 6, 2024

With this many iterations it is not surprising that it has such an impact. Perhaps in the future we can have session ID-based authentication for management UI, whenever we get to modernise it, or earlier if a good solution is figured out.

The PR looks fine but needs tests.

Copy link
Member

@michaelklishin michaelklishin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please drop the _v1 suffix, make the number of iterations configurable and provide some numbers on what the impact on CPU consumption and opening, say, of 50K connections is.

Connection churn is not something we recommend but it is not at all unheard of. 210K iterations may be really excessive but we should not be guessing as to what the impact is.


-module(rabbit_password_hashing_pbkdf2_hmac_sha512_v1).

%% TODO: I don't know if I should extend this behaviour, or change the other
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Behaviors can have optional callbacks. Changing other implementations does not seem to be necessary.

@michaelklishin michaelklishin changed the title wip: Implement PBKDF2 auth WIP A PBKDF2 implementation for internal database password hashing Sep 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

RabbitMQ should support pbkdf2 for password hashing
4 participants