Navigation Menu

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: make the bcrypt cost configurable #74582

Merged
merged 2 commits into from Jan 10, 2022

Conversation

knz
Copy link
Contributor

@knz knz commented Jan 7, 2022

Informs #74511.
(Do we want also to close that linked issue? I plan to rebase #74301 on top of this and follow the same pattern with a cluster setting.)

Release note (security update): For context, when configuring
passwords for SQL users, if the client presents the password in
cleartext via ALTER/CREATE USER/ROLE WITH PASSWORD, CockroachDB is
responsible for hashing this password before storing it.
By default, this hashing uses CockroachDB's bespoke crdb-bcrypt
algorithm, itself based off the standard Bcrypt algorithm.

The cost of this hashing function is now configurable via the new
cluster setting
server.user_login.password_hashes.default_cost.crdb_bcrypt.
Its default value is 10, which corresponds to an approximate
password check latency of 50-100ms on modern hardware.

This value should be increased over time to reflect improvements to
CPU performance: the latency should not become so small that it
becomes feasible to bruteforce passwords via repeated login attempts.

Future versions of CockroachDB will likely update the default accordingly.

Release note (security update): For context, when configuring
passwords for SQL users, if the client presents the password in
cleartext via ALTER/CREATE USER/ROLE WITH PASSWORD, CockroachDB is
responsible for hashing this password before storing it.
By default, this hashing uses CockroachDB's bespoke `crdb-bcrypt`
algorithm, itself based off the standard Bcrypt algorithm.

The cost of this hashing function is now configurable via the new
cluster setting
`server.user_login.password_hashes.default_cost.crdb_bcrypt`.
Its default value is 10, which corresponds to an approximate
password check latency of 50-100ms on modern hardware.

This value should be increased over time to reflect improvements to
CPU performance: the latency should not become so small that it
becomes feasible to bruteforce passwords via repeated login attempts.

Future versions of CockroachDB will likely update the default accordingly.
… gen

This cluster setting was meant to be exported for visibility in
auto-generated docs (we've documented it before). This was an oversight.

Release note: None
@knz knz requested review from bdarnell and aaron-crl January 7, 2022 21:19
@knz knz added this to In progress in DB Server & Security via automation Jan 7, 2022
@knz knz requested a review from a team as a code owner January 7, 2022 21:19
@knz knz requested a review from a team January 7, 2022 21:19
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@knz
Copy link
Contributor Author

knz commented Jan 7, 2022

Copy-pasting what I wrote elsewhere:

I'd like to point out that the likely reason why the default bcrypt 10 is still as good today as it was 6 years ago is that ... dennard scaling is dead and CPUs haven't meaningfully become faster.
(NB: looking through the lens of mandating a minimum login latency, to prevent bruteforcing passwords through a login page. Obviously, bruteforcing using a GPU given access to the raw hash is a different story.).
I don't see this changing anytime soon. This makes me question whether we should make this configurable at all.

@knz
Copy link
Contributor Author

knz commented Jan 7, 2022

Hm it may be worth lowering the value if running CockroachDB on a CPU that's significantly slower (e.g. an old Raspberri Pi). But is that really a use case we care about?

@knz knz requested review from kpatron-cockroachlabs and removed request for aaron-crl January 7, 2022 22:00
@knz
Copy link
Contributor Author

knz commented Jan 7, 2022

@kpatron-cockroachlabs , regarding whether this PR is desirable:

My personal view is a mild support for it mostly due to relative ease of implementation. As you say if we want to defend against ASICs or whatever bcrypt will get untenable in the long term but changing hashes would require more work and isn't a short or medium term concern

Copy link
Member

@bdarnell bdarnell left a comment

Choose a reason for hiding this comment

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

Obviously, bruteforcing using a GPU given access to the raw hash is a different story.

This is exactly the reason we use bcrypt (and equivalents) - to mitigate the risk of the raw hashes being compromised. If we just wanted to prevent online password guesses, we wouldn't need to burn CPU cycles with repeated hashing, we could use rate limiting, sleeps, etc.

Hm it may be worth lowering the value if running CockroachDB on a CPU that's significantly slower (e.g. an old Raspberri Pi). But is that really a use case we care about?

The problem with this reasoning is that the attacker's hardware matters too. If you managed to run a service on a raspberry pi that is important enough that someone's going to try and crack its passwords on GPUs/ASICs, you may have to choose a cost level that keeps them out even if it means your logins are quite slow. (but the server's hardware does still matter some - a more powerful server may choose a higher cost factor just to give itself more headroom at relatively little cost, while a less powerful one may want to do just enough to keep brute force infeasible, or compensate for a lower cost factor with a higher minimum password length).

This makes me question whether we should make this configurable at all.

We should make the SCRAM iteration count configurable. Since we have a relatively short time left before bcrypt is phased out in favor of SCRAM, I don't know that we need to make it configurable, but it's also very easy to do.

@knz
Copy link
Contributor Author

knz commented Jan 10, 2022

That works. Thanks ben!

bors r=bdarnell

@craig
Copy link
Contributor

craig bot commented Jan 10, 2022

Build succeeded:

@craig craig bot merged commit abd08d0 into cockroachdb:master Jan 10, 2022
DB Server & Security automation moved this from In progress to Done 21.2 Jan 10, 2022
@knz knz deleted the 20220107-pw-complexity branch January 10, 2022 18:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
DB Server & Security
  
Done 21.2
Development

Successfully merging this pull request may close these issues.

None yet

3 participants