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 hashed user passwords #103

Merged
merged 5 commits into from
Jan 17, 2023
Merged

Conversation

hoxu
Copy link
Contributor

@hoxu hoxu commented Dec 29, 2022

This partially reverts commit 004a620, that added MD5 hashing of all passwords on the provider side.
Now the passwords are again passed to Redshift as-is.
This allows Redshift to figure out whether the password is already hashed or needs to be hashed.
As a result, user passwords can be passed as hashed in the Terraform sources.

Unfortunately I could not run the acceptance tests against a Redshift cluster. Could someone help with the testing?

Fixes #97. Closes #90 and #96.

@hoxu
Copy link
Contributor Author

hoxu commented Dec 29, 2022

@sworisbreathing you authored the commit 004a620. It seemed to contain multiple changes, so can you review that this PR reverts only the relevant part?

@mtesch-um as author of #97, could you also review this?

Copy link
Contributor

@sworisbreathing sworisbreathing left a comment

Choose a reason for hiding this comment

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

@sworisbreathing you authored the commit 004a620. It seemed to contain multiple changes, so can you review that this PR reverts only the relevant part?

@hoxu confirmed, this only reverts the md5 hashing of the username

@mtesch-um
Copy link
Contributor

mtesch-um commented Dec 29, 2022

@hoxu thanks for pulling this together!

To prevent any regression here, I'd suggest adding tests to redshift/resource_redshift_user_test.go that exercise setting and altering the password using an "md5..." string in addition to a plaintext password.

@hoxu
Copy link
Contributor Author

hoxu commented Dec 30, 2022

I still can't run the tests, but commit 3bfdd33 is a blind-coded attempt at adding a test for creation of a user with MD5-hashed password.

@mtesch-um can you check how it looks like?

@hoxu hoxu requested a review from mtesch-um January 5, 2023 07:23
Copy link
Contributor

@mtesch-um mtesch-um left a comment

Choose a reason for hiding this comment

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

lgtm.

@mtesch-um
Copy link
Contributor

Also suggest the following addition to the documentation (docs/resources/user.md):

- **password** (String, Sensitive) Sets the user's password. Users can change their own passwords, unless the password is disabled. To disable password, omit this parameter or set it to `null`.  This can also be a md5-hashed password rather than the actual password.  Please refer to the [CREATE USER documentation](https://docs.aws.amazon.com/redshift/latest/dg/r_CREATE_USER.html) for information on creating an md5 password hash.  This value can NOT be a `sha256hash` string because it is also used in [ALTER USER which does not support that format](https://docs.aws.amazon.com/redshift/latest/dg/r_ALTER_USER.html).

@hoxu
Copy link
Contributor Author

hoxu commented Jan 10, 2023

@mtesch-um:

Documentation added in commit 49aad90.

Did you check that ALTER USER does not actually support SHA256, and it's just not missing from the documentation?

Can you review this one more time and resolve the conversations if you are satisfied? I'll rebase -i --autosquash after that.

@hoxu hoxu requested a review from mtesch-um January 10, 2023 07:59
@hoxu
Copy link
Contributor Author

hoxu commented Jan 10, 2023

I tested with Redshift and it seems ALTER USER at least supports the SHA256 syntax (the password is password in this example):

redacted=# BEGIN; CREATE USER delete_me PASSWORD 'sha256|1ab974118c07c09efe65e8bd04b0b47efaf232e2ee6f6132bd018ef72b208991|12e6c1c1cd63ef012278f51e5d65441930250d4701fd52274f6c5c1d1da35fad'; ALTER USER delete_me PASSWORD 'sha256|1ab974118c07c09efe65e8bd04b0b47efaf232e2ee6f6132bd018ef72b208991|12e6c1c1cd63ef012278f51e5d65441930250d4701fd52274f6c5c1d1da35fad'; ROLLBACK;
BEGIN
CREATE USER
ALTER USER
ROLLBACK

Creation of SHA256-hashed Redshift passwords is poorly documented in AWS documentation, but this Stack Overflow answer has instructions: https://stackoverflow.com/questions/73489343/creating-a-redshift-user-with-a-sha256-password

However, I didn't test logging in with a SHA256-hashed password, because apparently psql does not support it and fails with authentication method 13 not supported: https://www.repost.aws/questions/QUGYGra6yiQrOxXat9-tLcsg/redshift-error-authentication-method-13-not-supported-when-creating-a-user-with-sha-256-hashed-password

@hoxu
Copy link
Contributor Author

hoxu commented Jan 10, 2023

One more change, squash baee2bd. I removed mention of MD5 and SHA256 as well as ALTER USER link, and just mentioned that the password can be hashed, and to consult the CREATE USER documentation for more information.

@hoxu
Copy link
Contributor Author

hoxu commented Jan 12, 2023

I did git rebase -i --autosquash origin/master, and resolved the conversations, hope that's okay. Thakns for review and coding help @mtesch-um!

Could someone with write access, like @robertomczak, @rg00d, @szemek or @winglot review and approve this so that the test workflow can be executed?

docs/resources/user.md Outdated Show resolved Hide resolved
hoxu and others added 5 commits January 17, 2023 07:27
Commit 004a620

Pass the password as-is to Redshift, and let it figure out whether the
password is already hashed or needs to be hashed.

Allow providing passwords as hashed instead of only plaintext.
@winglot winglot merged commit 4bbf628 into brainly:master Jan 17, 2023
@mtesch-um
Copy link
Contributor

@hoxu thanks [again] for taking the initiative to fix this!!!

@hoxu
Copy link
Contributor Author

hoxu commented Feb 1, 2023

@mtesch-um Thanks to you for helping on every step along the way :)

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.

can not set a user password to an md5 string, because it will be re-hashed
4 participants