-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
sql: pull password hashing back to the sql
package
#51501
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @RichardJCai)
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wait shouldn't this return hashedPassword or a return inside the if statement?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand your question. Can you rephrase?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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