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

Support PasswordHash in User type #886

Merged
merged 7 commits into from
Sep 30, 2024
Merged

Conversation

NikSays
Copy link
Contributor

@NikSays NikSays commented Sep 14, 2024

This closes #865, #866

Note to reviewers: remember to look at the commits in this PR and consider if they can be squashed
Note to contributors: remember to re-generate client set if there are any API changes

Summary Of Changes

The Secret specified in User's ImportCredentialsSecret may contain the passwordHash field.
If the field is absent, the behavior is unchanged: the plain-text password is generated if necessary and stored in the credentials Secret.

If the passwordHash field is present, then password field is ignored and the resulting credentials Secret will contain only the hash. This is done to prevent ambiguous situations where the hash doesn't correspond to the password. It also prevents the plain-text password being accidentally used over the hash.

If the hash is an empty string, a passwordless user is created.

Additional Context

Currently the hash must be generated with the SHA512 algorithm. A way to specify another hashing algorithm needs to be added. I will add it as another field in the secret, but I'm concerned the documentation for importCredentialsSecret will become too complicated.

Todo

  • Specify hashing algorithm
  • Add examples
  • Update documentation

@vmwclabot
Copy link

@NikSays, you must sign our contributor license agreement before your changes are merged. Click here to sign the agreement. If you are a VMware employee, read this for further instruction.

@vmwclabot
Copy link

@NikSays, we have received your signed contributor license agreement. The review is usually completed within a week, but may take longer under certain circumstances. Another comment will be added to the pull request to notify you when the merge can proceed.

@Zerpet Zerpet self-assigned this Sep 19, 2024
@Zerpet Zerpet self-requested a review September 19, 2024 16:57
@Zerpet Zerpet added this to the v1.14.3 milestone Sep 19, 2024
Copy link
Contributor

@Zerpet Zerpet left a comment

Choose a reason for hiding this comment

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

I had a quick look and it looks good overall. I want to review again the system test changes in more detail tomorrow.

controllers/user_controller.go Outdated Show resolved Hide resolved
@Zerpet
Copy link
Contributor

Zerpet commented Sep 19, 2024

Thank you for contributing this feature. I like the high level logic proposed.

[...]. A way to specify another hashing algorithm needs to be added. [...]

I'm not sure we need to, can you elaborate on this?

If `passwordHash` field is set, then `password` field is ignored and the resulting credential secret will contain only the hash. A passwordless user is created if the hash is an empty string.
1. Create a User with a hash and verify that the user exists, and can be authenticated.
2. Create a User with an empty hash and verify that the password was ignored, the user exists, and cannot be authenticated.
Unset username was handled correctly, but empty username resulted in an unclear error: "Error 405 (Error 405 from RabbitMQ: EOF):".
Rename user_settings.go to user.go to reflect its extended scope
userWithPasswordHash.yaml - Creating a User with SHA-512 hash.
passwordlessUser.yaml - Creating a User with empty hash. Password field is ignored, paswordless user is created.
Describe the new structure of the `importCredentialsSecret` field
@NikSays
Copy link
Contributor Author

NikSays commented Sep 23, 2024

I'm not sure we need to, can you elaborate on this?

I've realized that forcing SHA-512 is viable. It won't work only if someone needs to create the User with a SHA-256 hash without access to the original password, or if someone wants to use a custom hashing function provided by a plugin. Both scenarios seem unlikely.
Implementing algorithm selection would require more effort, so until someone requests it or provides a use-case, SHA-512 should be enough.

@NikSays
Copy link
Contributor Author

NikSays commented Sep 23, 2024

I've added unit and system tests, updated the documentation, regenerated the asciidoc and the CRD. Also the branch was rebased on top of main, to simplify the commit history.

@NikSays NikSays marked this pull request as ready for review September 23, 2024 22:11
@NikSays NikSays changed the title Draft: Support PasswordHash in User type Support PasswordHash in User type Sep 23, 2024
@Zerpet
Copy link
Contributor

Zerpet commented Sep 24, 2024

The latest changes look good to me. I'll merge after CI passes 🚀

@NikSays
Copy link
Contributor Author

NikSays commented Sep 30, 2024

Hi, @Zerpet, sorry to bother you. Could you please restart the CI workflow, there was a timeout starting the cluster operator, seems like a one-off issue.

@Zerpet Zerpet merged commit 3a1acbc into rabbitmq:main Sep 30, 2024
7 checks passed
@Zerpet
Copy link
Contributor

Zerpet commented Sep 30, 2024

Thank you!

@Zerpet Zerpet linked an issue Sep 30, 2024 that may be closed by this pull request
Zerpet added a commit to Zerpet/rabbitmq-website that referenced this pull request Oct 14, 2024
Another feature that was implemented and released, but never documented.
Related to rabbitmq/messaging-topology-operator/pull/886
@vmwclabot
Copy link

@NikSays, VMware has approved your signed contributor license agreement.

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.

Support PasswordHash in User type Creating a passwordless user
3 participants