-
Notifications
You must be signed in to change notification settings - Fork 48
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
can not set a user password to an md5 string, because it will be re-hashed #97
Comments
This was broken in 004a620. I added this comment on PR #90:
Commit 004a620 certainly does not provide any explanation for why the MD5 hashing in the provider code side was added. @sworisbreathing what was the reasoning? |
Commit 004a620 went into the v0.2.0 release, PR #7. That PR only quotes the
The "more secure" part applies when comparing, for example, a SQL migration file with a plaintext password versus one with a hashed version of the same. I don't see any security gained from doing the hashing in the provider execution - the connection is encrypted unless you explicitly disable that. In fact, the current MD5 hashing may be inferior to whatever (stronger) hashing Redshift does now or in the future. I propose the MD5-hashing in 004a620 to be reverted, and instead the plain text password string passed as-is to Redshift. Redshift then either hashes the password using whatever is the current default hashing algorithm, or validates an already hashed password and stores it. Simpler provider code, and no need to maintain an implementation containing a separate, hardcoded, eventually outdated, list of supported hash algorithms with inferior validation logic. @sworisbreathing would you accept a PR reverting that MD5-hashing and passing the password as-is to Redshift, as before? |
@hoxu it probably should be configurable rather than forcing any of the options. I think I made the original PR to go with md5 because I was concerned about a) passwords being written in plaintext to query logs, and b) plaintext passwords being stored in general. I probably would have used sha256 instead of md5 had I been aware that redshift supported it at the time. I was never a big fan of md5 but it seemed better than nothing. Unfortunately I can't accept any PRs. I'm not a maintainer on this project, only a contributor like yourself. |
I would guess the reason to hash is to keep the unhashed password from passing over the wire unencrypted - probably a minor concern for most users, since the connection should be SSL anyhow.
I dug into using sha256 at one point - if I'm remembering correctly, sha256 is only accepted for CREATE USER and not for ALTER USER. |
We really need the ability to pass hashed passwords, or else we need to use something else than this provider. Can someone with commit rights (@rg00d, @szemek, @winglot?) chime in and comment on the proposed solutions?
I may be able to work on a simple PR, but let's agree on what's the correct way to fix this first. |
fwiw, my vote is for solution 1. |
PR #103 created with proposed solution 1. I hope someone with write access can review it. |
This used to work, but has been broken by always hashing the password string before sending it to CREATE USER.
There are now two PRs to fix this - I did the second one because I forgot there was already one:
#90
#96
The text was updated successfully, but these errors were encountered: