sql: pull password hashing back to the sql package#51501
sql: pull password hashing back to the sql package#51501craig[bot] merged 1 commit intocockroachdb:masterfrom
sql package#51501Conversation
|
This change is |
RichardJCai
left a comment
There was a problem hiding this comment.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @knz and @RichardJCai)
pkg/sql/alter_role.go, line 148 at r1 (raw file):
} if hashedPassword == nil {
Nit: Does this logic for setting hashedPassword to an empty byte array make sense to move into checkPasswordAndGetHash? We can avoid duplication by doing so.
This also removes some obsolete code, and ensures that the username validity check is performed before the password validity check. Release note: None
knz
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/sql/alter_role.go, line 148 at r1 (raw file):
Previously, RichardJCai (Richard Cai) wrote…
Nit: Does this logic for setting hashedPassword to an empty byte array make sense to move into checkPasswordAndGetHash? We can avoid duplication by doing so.
Good idea! Done.
|
the CI is flaking on #51543 |
51502: security,sql: add a setting to constrain the minimum password length r=RichardJCai a=knz Requested by @bdarnell. First commit from #51501. This change adds a new cluster setting `server.user_login.min_password_length`. When set and non-zero, it forces a minimum password length to passwords passed in cleartext to the SQL `WITH PASSWORD` clause (`CREATE/ALTER USER/ROLE`). This change is part of a larger set of security measures to help the secure deployment of Cockroach Cloud. We are not yet planning to advertise this to on-prem users until the feature matures and is complemented by additional enhancements to password authentication. cc @solongordon @thtruo @aaron-crl for visibility. 51542: cli: temporarily skip TestPartialZip and CDC tests r=tbg a=knz Due to #51538 Release note: None Co-authored-by: Raphael 'kena' Poss <knz@thaumogen.net>
| hashedPassword = []byte{} | ||
| } | ||
|
|
||
| return |
There was a problem hiding this comment.
Wait shouldn't this return hashedPassword or a return inside the if statement?
There was a problem hiding this comment.
I don't understand your question. Can you rephrase?
There was a problem hiding this comment.
Ah nvm I see that the return values are named, ignore my comment.
Not a huge fan of named return values with an empty return statement though.
This also removes some obsolete code, and ensures that the username
validity check is performed before the password validity check.
Release note: None