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

security: allow automatic re-hashing from SCRAM to bcrypt #97429

Merged
merged 1 commit into from
Feb 24, 2023

Conversation

rafiss
Copy link
Collaborator

@rafiss rafiss commented Feb 21, 2023

fixes #96408

Release note (security update): Currently the process to change
passwords from SCRAM hashing to bcrypt hashing is manual and can take a
long time. An operator may wish to do this if their tools do not support
SCRAM or if SCRAM is not desired for another reason.
This change introduces the new cluster setting
server.user_login.downgrade_scram_stored_passwords_to_bcrypt.enabled,
which defaults to true. If it is true AND
server.user_login.password_encryption is crdb-bcrypt, then during
login, the stored hashed password will be converted from SCRAM to bcrypt.

@rafiss rafiss requested a review from knz February 21, 2023 23:24
@rafiss rafiss requested review from a team as code owners February 21, 2023 23:24
@rafiss rafiss requested a review from a team February 21, 2023 23:24
@cockroach-teamcity
Copy link
Member

This change is Reviewable

Copy link
Contributor

@knz knz left a comment

Choose a reason for hiding this comment

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

Very nice.

LGTM modulo code organization.

Reviewed 10 of 10 files at r1, all commit messages.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @rafiss)


-- commits line 4 at r1:
I'd recommend explaining the use case (why this is useful, when would a user want to use it etc) before the change.


-- commits line 5 at r1:
nit: add quotes around the setting names


pkg/security/password/password.go line 719 at r1 (raw file):

	// - conversion enabled by cluster setting.
	// - the configured default method is scram-sha-256.
	if isBcrypt && autoUpgradePasswordHashes && PasswordHashMethod == HashSCRAMSHA256 {

This function is becoming large and hard to read/maintain. I'd recommend splitting it up.


pkg/security/password/password.go line 719 at r1 (raw file):

	// - conversion enabled by cluster setting.
	// - the configured default method is scram-sha-256.
	if isBcrypt && autoUpgradePasswordHashes && PasswordHashMethod == HashSCRAMSHA256 {

The isBcrypt/isScram bools can be replaced by a type switch.

Release note (security update): Currently the process to change
passwords from SCRAM hashing to bcrypt hashing is manual and can take a
long time. An operator may wish to do this if their tools do not support
SCRAM or if SCRAM is not desired for another reason.
This change introduces the new cluster setting
`server.user_login.downgrade_scram_stored_passwords_to_bcrypt.enabled`,
which defaults to true. If it is true AND
`server.user_login.password_encryption` is `crdb-bcrypt`, then during
login, the stored hashed password will be converted from SCRAM to bcrypt.
@rafiss rafiss force-pushed the downgrade-to-bcrypt branch from b0256b6 to 3d2491a Compare February 23, 2023 22:56
@rafiss
Copy link
Collaborator Author

rafiss commented Feb 23, 2023

RFAL. For some reason, I can't reply to your comments inline (nor see them inline). I extracted some of the common logic, and adjusted the release note. I don't feel like a type switch improves readability, since we also need to check those other bools.

@rafiss rafiss requested a review from knz February 23, 2023 22:57
Copy link
Contributor

@knz knz left a comment

Choose a reason for hiding this comment

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

Reviewed 10 of 10 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @rafiss)


-- commits line 4 at r1:
I'd recommend explaining the use case (why this is useful, when would a user want to use it etc) before the change.


-- commits line 5 at r1:
nit: add quotes around the setting names

@knz
Copy link
Contributor

knz commented Feb 24, 2023

sorry i don't know why my comments were duplicated. please ignore

@rafiss
Copy link
Collaborator Author

rafiss commented Feb 24, 2023

tftr! flake in Extended CI was #96343

bors r=knz

@craig
Copy link
Contributor

craig bot commented Feb 24, 2023

Build succeeded:

@craig craig bot merged commit ad3c5d5 into cockroachdb:master Feb 24, 2023
@blathers-crl
Copy link

blathers-crl bot commented Feb 24, 2023

Encountered an error creating backports. Some common things that can go wrong:

  1. The backport branch might have already existed.
  2. There was a merge conflict.
  3. The backport branch contained merge commits.

You might need to create your backport manually using the backport tool.


error creating merge commit from 3d2491a to blathers/backport-release-22.2-97429: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict []

you may need to manually resolve merge conflicts with the backport tool.

Backport to branch 22.2.x failed. See errors above.


🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.

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.

sql: add a cluster setting to easily roll back SCRAM
3 participants