Merge #74582

74582: security: make the bcrypt cost configurable r=bdarnell a=knz

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.

Co-authored-by: Raphael 'kena' Poss <[email protected]>

docs/generated/settings/settings-for-tenants.txt

@@ -68,6 +68,8 @@ server.shutdown.drain_wait	duration	0s	the amount of time a server waits in an u
 server.shutdown.lease_transfer_wait	duration	5s	the amount of time a server waits to transfer range leases before proceeding with the rest of the shutdown process (note that the --drain-wait parameter for cockroach node drain may need adjustment after changing this setting)
 server.shutdown.query_wait	duration	10s	the server will wait for at least this amount of time for active queries to finish (note that the --drain-wait parameter for cockroach node drain may need adjustment after changing this setting)
 server.time_until_store_dead	duration	5m0s	the time after which if there is no new gossiped information about a store, it is considered dead
+server.user_login.min_password_length	integer	1	the minimum length accepted for passwords set in cleartext via SQL. Note that a value lower than 1 is ignored: passwords cannot be empty in any case.
+server.user_login.password_hashes.default_cost.crdb_bcrypt	integer	10	the hashing cost to use when storing passwords supplied as cleartext by SQL clients with the hashing method crdb-bcrypt (allowed range: 4-31)
 server.user_login.store_client_pre_hashed_passwords.enabled	boolean	true	whether the server accepts to store passwords pre-hashed by clients
 server.user_login.timeout	duration	10s	timeout after which client authentication times out if some system range is unavailable (0 = no timeout)
 server.web_session.auto_logout.timeout	duration	168h0m0s	the duration that web sessions will survive before being periodically purged, since they were last used

docs/generated/settings/settings.html

@@ -74,6 +74,8 @@
 <tr><td><code>server.shutdown.lease_transfer_wait</code></td><td>duration</td><td><code>5s</code></td><td>the amount of time a server waits to transfer range leases before proceeding with the rest of the shutdown process (note that the --drain-wait parameter for cockroach node drain may need adjustment after changing this setting)</td></tr>
 <tr><td><code>server.shutdown.query_wait</code></td><td>duration</td><td><code>10s</code></td><td>the server will wait for at least this amount of time for active queries to finish (note that the --drain-wait parameter for cockroach node drain may need adjustment after changing this setting)</td></tr>
 <tr><td><code>server.time_until_store_dead</code></td><td>duration</td><td><code>5m0s</code></td><td>the time after which if there is no new gossiped information about a store, it is considered dead</td></tr>
+<tr><td><code>server.user_login.min_password_length</code></td><td>integer</td><td><code>1</code></td><td>the minimum length accepted for passwords set in cleartext via SQL. Note that a value lower than 1 is ignored: passwords cannot be empty in any case.</td></tr>
+<tr><td><code>server.user_login.password_hashes.default_cost.crdb_bcrypt</code></td><td>integer</td><td><code>10</code></td><td>the hashing cost to use when storing passwords supplied as cleartext by SQL clients with the hashing method crdb-bcrypt (allowed range: 4-31)</td></tr>
 <tr><td><code>server.user_login.store_client_pre_hashed_passwords.enabled</code></td><td>boolean</td><td><code>true</code></td><td>whether the server accepts to store passwords pre-hashed by clients</td></tr>
 <tr><td><code>server.user_login.timeout</code></td><td>duration</td><td><code>10s</code></td><td>timeout after which client authentication times out if some system range is unavailable (0 = no timeout)</td></tr>
 <tr><td><code>server.web_session.auto_logout.timeout</code></td><td>duration</td><td><code>168h0m0s</code></td><td>the duration that web sessions will survive before being periodically purged, since they were last used</td></tr>

pkg/ccl/serverccl/role_authentication_test.go

@@ -42,11 +42,12 @@ func TestVerifyPassword(t *testing.T) {
 		s.ExecutorConfig().(sql.ExecutorConfig).Settings,
 	)
 
+	ts := s.(*server.TestServer)
+
 	if util.RaceEnabled {
 		// The default bcrypt cost makes this test approximately 30s slower when the
 		// race detector is on.
-		defer func(prev int) { security.BcryptCost = prev }(security.BcryptCost)
-		security.BcryptCost = bcrypt.MinCost
+		security.BcryptCost.Override(ctx, &ts.Cfg.Settings.SV, int64(bcrypt.MinCost))
 	}
 
 	//location is used for timezone testing.

pkg/security/password.go

@@ -14,6 +14,7 @@ import (
 	"bytes"
 	"context"
 	"crypto/sha256"
+	"fmt"
 	"regexp"
 	"runtime"
 	"sync"
@@ -28,13 +29,34 @@ import (
 	"golang.org/x/crypto/bcrypt"
 )
 
-// BcryptCost is the cost to use when hashing passwords. It is exposed for
-// testing.
+// BcryptCost is the cost to use when hashing passwords.
+// It is exposed for testing.
 //
-// BcryptCost should increase along with computation power.
-// For estimates, see: http://security.stackexchange.com/questions/17207/recommended-of-rounds-for-bcrypt
-// For now, we use the library's default cost.
-var BcryptCost = bcrypt.DefaultCost
+// The default value of BcryptCost should increase along with
+// computation power.
+//
+// For estimates, see:
+// http://security.stackexchange.com/questions/17207/recommended-of-rounds-for-bcrypt
+var BcryptCost = settings.RegisterIntSetting(
+	settings.TenantWritable,
+	BcryptCostSettingName,
+	fmt.Sprintf(
+		"the hashing cost to use when storing passwords supplied as cleartext by SQL clients "+
+			"with the hashing method crdb-bcrypt (allowed range: %d-%d)",
+		bcrypt.MinCost, bcrypt.MaxCost),
+	// The default value 10 is equal to bcrypt.DefaultCost.
+	// It incurs a password check latency of ~60ms on AMD 3950X 3.7GHz.
+	// For reference, value 11 incurs ~110ms latency on the same hw, value 12 incurs ~390ms.
+	10,
+	func(i int64) error {
+		if i < int64(bcrypt.MinCost) || i > int64(bcrypt.MaxCost) {
+			return bcrypt.InvalidCostError(int(i))
+		}
+		return nil
+	}).WithPublic()
+
+// BcryptCostSettingName is the name of the cluster setting BcryptCost.
+const BcryptCostSettingName = "server.user_login.password_hashes.default_cost.crdb_bcrypt"
 
 // ErrEmptyPassword indicates that an empty password was attempted to be set.
 var ErrEmptyPassword = errors.New("empty passwords are not permitted")
@@ -74,14 +96,14 @@ func CompareHashAndPassword(ctx context.Context, hashedPassword []byte, password
 }
 
 // HashPassword takes a raw password and returns a bcrypt hashed password.
-func HashPassword(ctx context.Context, password string) ([]byte, error) {
+func HashPassword(ctx context.Context, sv *settings.Values, password string) ([]byte, error) {
 	sem := getBcryptSem(ctx)
 	alloc, err := sem.Acquire(ctx, 1)
 	if err != nil {
 		return nil, err
 	}
 	defer alloc.Release()
-	return bcrypt.GenerateFromPassword(appendEmptySha256(password), BcryptCost)
+	return bcrypt.GenerateFromPassword(appendEmptySha256(password), int(BcryptCost.Get(sv)))
 }
 
 // AutoDetectPasswordHashes is the cluster setting that configures whether
@@ -168,7 +190,7 @@ var MinPasswordLength = settings.RegisterIntSetting(
 		"Note that a value lower than 1 is ignored: passwords cannot be empty in any case.",
 	1,
 	settings.NonNegativeInt,
-)
+).WithPublic()
 
 // bcryptSemOnce wraps a semaphore that limits the number of concurrent calls
 // to the bcrypt hash functions. This is needed to avoid the risk of a

pkg/server/authentication_test.go

@@ -201,8 +201,7 @@ func TestVerifyPassword(t *testing.T) {
 	if util.RaceEnabled {
 		// The default bcrypt cost makes this test approximately 30s slower when the
 		// race detector is on.
-		defer func(prev int) { security.BcryptCost = prev }(security.BcryptCost)
-		security.BcryptCost = bcrypt.MinCost
+		security.BcryptCost.Override(ctx, &ts.Cfg.Settings.SV, int64(bcrypt.MinCost))
 	}
 
 	//location is used for timezone testing.

pkg/sql/create_role.go

@@ -282,7 +282,7 @@ func (p *planner) checkPasswordAndGetHash(
 			"Passwords must be %d characters or longer.", minLength)
 	}
 
-	hashedPassword, err = security.HashPassword(ctx, password)
+	hashedPassword, err = security.HashPassword(ctx, &st.SV, password)
 	if err != nil {
 		return hashedPassword, err
 	}

pkg/sql/logictest/testdata/logic_test/role

@@ -1458,4 +1458,21 @@ hash6  $2a$10$  false
 
 # Reset cluster setting after test completion.
 statement ok
-SET CLUSTER SETTING server.user_login.store_client_pre_hashed_passwords.enabled = true
+RESET CLUSTER SETTING server.user_login.store_client_pre_hashed_passwords.enabled;
+
+subtest bcrypt_cost
+
+statement ok
+SET CLUSTER SETTING server.user_login.password_hashes.default_cost.crdb_bcrypt = 20
+
+statement ok
+CREATE USER hash7 WITH PASSWORD 'hello'
+
+# Check that the configured cost was embedded in the setting.
+query TT
+SELECT username, substr("hashedPassword", 1, 7) FROM system.users WHERE username = 'hash7'
+----
+hash7  $2a$20$
+
+statement ok
+RESET CLUSTER SETTING server.user_login.password_hashes.default_cost.crdb_bcrypt;