Merge #97429

97429: security: allow automatic re-hashing from SCRAM to bcrypt r=knz a=rafiss

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.

Co-authored-by: Rafi Shamim <[email protected]>

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

@@ -86,12 +86,13 @@ server.shutdown.lease_transfer_wait	duration	5s	the timeout for a single iterati
 server.shutdown.query_wait	duration	10s	the timeout for waiting for active queries to finish during a drain (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.cert_password_method.auto_scram_promotion.enabled	boolean	true	whether to automatically promote cert-password authentication to use SCRAM
+server.user_login.downgrade_scram_stored_passwords_to_bcrypt.enabled	boolean	true	if server.user_login.password_encryption=crdb-bcrypt, this controls whether to automatically re-encode stored passwords using scram-sha-256 to crdb-bcrypt
 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_encryption	enumeration	scram-sha-256	which hash method to use to encode cleartext passwords passed via ALTER/CREATE USER/ROLE WITH PASSWORD [crdb-bcrypt = 2, scram-sha-256 = 3]
 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.password_hashes.default_cost.scram_sha_256	integer	119680	the hashing cost to use when storing passwords supplied as cleartext by SQL clients with the hashing method scram-sha-256 (allowed range: 4096-240000000000)
 server.user_login.timeout	duration	10s	timeout after which client authentication times out if some system range is unavailable (0 = no timeout)
-server.user_login.upgrade_bcrypt_stored_passwords_to_scram.enabled	boolean	true	whether to automatically re-encode stored passwords using crdb-bcrypt to scram-sha-256
+server.user_login.upgrade_bcrypt_stored_passwords_to_scram.enabled	boolean	true	if server.user_login.password_encryption=scram-sha-256, this controls whether to automatically re-encode stored passwords using crdb-bcrypt to scram-sha-256
 server.web_session.purge.ttl	duration	1h0m0s	if nonzero, entries in system.web_sessions older than this duration are periodically purged
 server.web_session_timeout	duration	168h0m0s	the duration that a newly created web session will be valid
 sql.auth.change_own_password.enabled	boolean	false	controls whether a user is allowed to change their own password, even if they have no other privileges

docs/generated/settings/settings.html

@@ -107,12 +107,13 @@
 <tr><td><div id="setting-server-shutdown-query-wait" class="anchored"><code>server.shutdown.query_wait</code></div></td><td>duration</td><td><code>10s</code></td><td>the timeout for waiting for active queries to finish during a drain (note that the --drain-wait parameter for cockroach node drain may need adjustment after changing this setting)</td></tr>
 <tr><td><div id="setting-server-time-until-store-dead" class="anchored"><code>server.time_until_store_dead</code></div></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><div id="setting-server-user-login-cert-password-method-auto-scram-promotion-enabled" class="anchored"><code>server.user_login.cert_password_method.auto_scram_promotion.enabled</code></div></td><td>boolean</td><td><code>true</code></td><td>whether to automatically promote cert-password authentication to use SCRAM</td></tr>
+<tr><td><div id="setting-server-user-login-downgrade-scram-stored-passwords-to-bcrypt-enabled" class="anchored"><code>server.user_login.downgrade_scram_stored_passwords_to_bcrypt.enabled</code></div></td><td>boolean</td><td><code>true</code></td><td>if server.user_login.password_encryption=crdb-bcrypt, this controls whether to automatically re-encode stored passwords using scram-sha-256 to crdb-bcrypt</td></tr>
 <tr><td><div id="setting-server-user-login-min-password-length" class="anchored"><code>server.user_login.min_password_length</code></div></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><div id="setting-server-user-login-password-encryption" class="anchored"><code>server.user_login.password_encryption</code></div></td><td>enumeration</td><td><code>scram-sha-256</code></td><td>which hash method to use to encode cleartext passwords passed via ALTER/CREATE USER/ROLE WITH PASSWORD [crdb-bcrypt = 2, scram-sha-256 = 3]</td></tr>
 <tr><td><div id="setting-server-user-login-password-hashes-default-cost-crdb-bcrypt" class="anchored"><code>server.user_login.password_hashes.default_cost.crdb_bcrypt</code></div></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><div id="setting-server-user-login-password-hashes-default-cost-scram-sha-256" class="anchored"><code>server.user_login.password_hashes.default_cost.scram_sha_256</code></div></td><td>integer</td><td><code>119680</code></td><td>the hashing cost to use when storing passwords supplied as cleartext by SQL clients with the hashing method scram-sha-256 (allowed range: 4096-240000000000)</td></tr>
 <tr><td><div id="setting-server-user-login-timeout" class="anchored"><code>server.user_login.timeout</code></div></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><div id="setting-server-user-login-upgrade-bcrypt-stored-passwords-to-scram-enabled" class="anchored"><code>server.user_login.upgrade_bcrypt_stored_passwords_to_scram.enabled</code></div></td><td>boolean</td><td><code>true</code></td><td>whether to automatically re-encode stored passwords using crdb-bcrypt to scram-sha-256</td></tr>
+<tr><td><div id="setting-server-user-login-upgrade-bcrypt-stored-passwords-to-scram-enabled" class="anchored"><code>server.user_login.upgrade_bcrypt_stored_passwords_to_scram.enabled</code></div></td><td>boolean</td><td><code>true</code></td><td>if server.user_login.password_encryption=scram-sha-256, this controls whether to automatically re-encode stored passwords using crdb-bcrypt to scram-sha-256</td></tr>
 <tr><td><div id="setting-server-web-session-purge-ttl" class="anchored"><code>server.web_session.purge.ttl</code></div></td><td>duration</td><td><code>1h0m0s</code></td><td>if nonzero, entries in system.web_sessions older than this duration are periodically purged</td></tr>
 <tr><td><div id="setting-server-web-session-timeout" class="anchored"><code>server.web_session_timeout</code></div></td><td>duration</td><td><code>168h0m0s</code></td><td>the duration that a newly created web session will be valid</td></tr>
 <tr><td><div id="setting-sql-auth-change-own-password-enabled" class="anchored"><code>sql.auth.change_own_password.enabled</code></div></td><td>boolean</td><td><code>false</code></td><td>controls whether a user is allowed to change their own password, even if they have no other privileges</td></tr>

pkg/security/password.go

@@ -164,7 +164,18 @@ var MinPasswordLength = settings.RegisterIntSetting(
 var AutoUpgradePasswordHashes = settings.RegisterBoolSetting(
 	settings.TenantWritable,
 	"server.user_login.upgrade_bcrypt_stored_passwords_to_scram.enabled",
-	"whether to automatically re-encode stored passwords using crdb-bcrypt to scram-sha-256",
+	"if server.user_login.password_encryption=scram-sha-256, this controls "+
+		"whether to automatically re-encode stored passwords using crdb-bcrypt to scram-sha-256",
+	true,
+).WithPublic()
+
+// AutoDowngradePasswordHashes is the cluster setting that configures whether to
+// automatically re-encode stored passwords using scram-sha-256 to crdb-bcrypt.
+var AutoDowngradePasswordHashes = settings.RegisterBoolSetting(
+	settings.TenantWritable,
+	"server.user_login.downgrade_scram_stored_passwords_to_bcrypt.enabled",
+	"if server.user_login.password_encryption=crdb-bcrypt, this controls "+
+		"whether to automatically re-encode stored passwords using scram-sha-256 to crdb-bcrypt",
 	true,
 ).WithPublic()
 

pkg/security/password/password.go

@@ -679,89 +679,158 @@ var BcryptCostToSCRAMIterCount = []int64{
 	238691162884, // 31
 }
 
-// MaybeUpgradePasswordHash looks at the cleartext and the hashed
-// password and determines whether the hash can be upgraded from
-// crdb-bcrypt to scram-sha-256. If it can, it computes an equivalent
-// SCRAM hash and returns it.
+// ScramIterCountToBcryptCost computes the inverse of the
+// BcryptCostToSCRAMIterCount mapping.
+func ScramIterCountToBcryptCost(scramIters int) int {
+	for i, thisIterCount := range BcryptCostToSCRAMIterCount {
+		if i >= bcrypt.MaxCost {
+			return bcrypt.MaxCost
+		}
+		if int64(scramIters) >= thisIterCount && int64(scramIters) < BcryptCostToSCRAMIterCount[i+1] {
+			return i
+		}
+	}
+	return 0
+}
+
+// MaybeConvertPasswordHash looks at the cleartext and the hashed
+// password and determines whether the hash can be converted from/to
+// crdb-bcrypt to/from scram-sha-256. If it can, it computes an equivalent
+// hash and returns it.
 //
 // See the documentation on BcryptCostToSCRAMIterCount[] for details.
-func MaybeUpgradePasswordHash(
+func MaybeConvertPasswordHash(
 	ctx context.Context,
 	autoUpgradePasswordHashes bool,
-	PasswordHashMethod HashMethod,
+	autoDowngradePasswordHashesBool bool,
+	configuredHashMethod HashMethod,
 	cleartext string,
 	hashed PasswordHash,
 	hashSem HashSemaphore,
 	logEvent func(context.Context, string, ...interface{}),
 ) (converted bool, prevHashBytes, newHashBytes []byte, newMethod string, err error) {
 	bh, isBcrypt := hashed.(bcryptHash)
+	sh, isScram := hashed.(*scramHash)
+
+	// First check upgrading hashes. We need the following:
+	// - password currently hashed using crdb-bcrypt.
+	// - conversion enabled by cluster setting.
+	// - the configured default method is scram-sha-256.
+	if isBcrypt && autoUpgradePasswordHashes && configuredHashMethod == HashSCRAMSHA256 {
+		bcryptCost, err := bcrypt.Cost(bh)
+		if err != nil {
+			// The caller should only call this function after authentication
+			// has succeeded, so the bcrypt cost should have been validated
+			// already.
+			return false, nil, nil, "", errors.NewAssertionErrorWithWrappedErrf(err, "programming error: authn succeeded but invalid bcrypt hash")
+		}
+		if bcryptCost < bcrypt.MinCost || bcryptCost > bcrypt.MaxCost || BcryptCostToSCRAMIterCount[bcryptCost] == 0 {
+			// The bcryptCost was smaller than 4 or greater than 31? That's a violation of a bcrypt invariant.
+			// Or perhaps the BcryptCostToSCRAMIterCount was incorrectly modified and there's a hole with value zero.
+			return false, nil, nil, "", errors.AssertionFailedf("unexpected: bcrypt cost %d is out of bounds or has no mapping", bcryptCost)
+		}
 
-	// Do we want to perform the conversion?
-	//
-	// We stop here in the following cases:
-	// - password not currently hashed using crdb-bcrypt: we can't convert.
-	// - conversion disabled by cluster setting.
-	// - some nodes don't know about SCRAM just yet during an upgrade.
-	//   (checked in GetConfiguredPasswordHashMethod)
-	// - the configured default method is not scram-sha-256.
-	if !isBcrypt || !autoUpgradePasswordHashes ||
-		PasswordHashMethod != HashSCRAMSHA256 {
-		// Nothing to do.
-		return false, nil, nil, "", nil
-	}
-
-	bcryptCost, err := bcrypt.Cost([]byte(bh))
-	if err != nil {
-		// The caller should only call this function after authentication
-		// has succeeded, so the bcrypt cost should have been validated
-		// already.
-		return false, nil, nil, "", errors.NewAssertionErrorWithWrappedErrf(err, "programming error: authn succeeded but invalid bcrypt hash")
-	}
-	if bcryptCost < 0 || bcryptCost >= len(BcryptCostToSCRAMIterCount) || BcryptCostToSCRAMIterCount[bcryptCost] == 0 {
-		// The bcryptCost was smaller than 4 or greater than 31? That's a violation of a bcrypt invariant.
-		// Or perhaps the BcryptCostToSCRAMIterCount was incorrectly modified and there's a hole with value zero.
-		return false, nil, nil, "", errors.AssertionFailedf("unexpected: bcrypt cost %d is out of bounds or has no mapping", bcryptCost)
-	}
-
-	scramIterCount := BcryptCostToSCRAMIterCount[bcryptCost]
-	if scramIterCount > math.MaxInt {
-		// scramIterCount is an int64. However, the SCRAM library we're using (xdg/scram) uses
-		// an int for the iteration count. This is not an issue when this code is running
-		// on a 64-bit platform, where sizeof(int) == sizeof(int64). However, when running
-		// on 32-bit, we can't allow a conversion to proceed because it would potentially
-		// truncate the iter count to a low value.
-		// However, this situation is not an error. The hash could still be converted later
-		// when the system is upgraded to 64-bit.
-		return false, nil, nil, "", nil
-	}
-
-	if bcryptCost > 10 {
-		// Tell the logs that we're doing a conversion. This is important
-		// if the new cost is high and the operation takes a long time, so
-		// the operator knows what's up.
-		//
-		// Note: this is an informational message for troubleshooting
-		// purposes, and therefore is best sent to the DEV channel. The
-		// structured event that reports that the conversion has completed
-		// (and the new credentials were stored) is sent by the SQL code
-		// that also owns the storing of the new credentials.
-		logEvent(ctx, "hash conversion: computing a SCRAM hash with iteration count %d (from bcrypt cost %d)", scramIterCount, bcryptCost)
+		scramIterCount := BcryptCostToSCRAMIterCount[bcryptCost]
+		if scramIterCount > math.MaxInt {
+			// scramIterCount is an int64. However, the SCRAM library we're using (xdg/scram) uses
+			// an int for the iteration count. This is not an issue when this code is running
+			// on a 64-bit platform, where sizeof(int) == sizeof(int64). However, when running
+			// on 32-bit, we can't allow a conversion to proceed because it would potentially
+			// truncate the iter count to a low value.
+			// However, this situation is not an error. The hash could still be converted later
+			// when the system is upgraded to 64-bit.
+			return false, nil, nil, "", nil
+		}
+
+		if bcryptCost > 10 {
+			// Tell the logs that we're doing a conversion. This is important
+			// if the new cost is high and the operation takes a long time, so
+			// the operator knows what's up.
+			//
+			// Note: this is an informational message for troubleshooting
+			// purposes, and therefore is best sent to the DEV channel. The
+			// structured event that reports that the conversion has completed
+			// (and the new credentials were stored) is sent by the SQL code
+			// that also owns the storing of the new credentials.
+			logEvent(ctx, "hash conversion: computing a SCRAM hash with iteration count %d (from bcrypt cost %d)", scramIterCount, bcryptCost)
+		}
+
+		newHash, err := hashAndValidate(ctx, int(scramIterCount), HashSCRAMSHA256, cleartext, hashSem)
+		if err != nil {
+			// This call only fail with hard errors.
+			return false, nil, nil, "", err
+		}
+		return true, bh, newHash, HashSCRAMSHA256.String(), nil
+	}
+
+	// Now check downgrading hashes. We need the following:
+	// - password currently hashed using scram-sha-256.
+	// - conversion enabled by cluster setting.
+	// - the configured default method is crdb-bcrypt.
+	if isScram && autoDowngradePasswordHashesBool && configuredHashMethod == HashBCrypt {
+		ok, parts := isSCRAMHash(sh.bytes)
+		if !ok {
+			// The caller should only call this function after authentication
+			// has succeeded, so the scram hash should have been validated
+			// already.
+			return false, nil, nil, "", errors.AssertionFailedf("programming error: authn succeeded but invalid scram hash")
+		}
+		scramIters, err := parts.getIters()
+		if err != nil {
+			// The caller should only call this function after authentication
+			// has succeeded, so the scram iters should have been validated
+			// already.
+			return false, nil, nil, "", errors.NewAssertionErrorWithWrappedErrf(err, "programming error: authn succeeded but invalid scram hash")
+		}
+		if scramIters < ScramMinCost || scramIters > ScramMaxCost {
+			// The scramIters being out of range is a violation of our SCRAM preconditions.
+			return false, nil, nil, "", errors.AssertionFailedf("unexpected: scram iteration count %d is out of bounds", scramIters)
+		}
+
+		bcryptCost := ScramIterCountToBcryptCost(scramIters)
+		if bcryptCost > 10 {
+			// Tell the logs that we're doing a conversion. This is important
+			// if the new cost is high and the operation takes a long time, so
+			// the operator knows what's up.
+			//
+			// Note: this is an informational message for troubleshooting
+			// purposes, and therefore is best sent to the DEV channel. The
+			// structured event that reports that the conversion has completed
+			// (and the new credentials were stored) is sent by the SQL code
+			// that also owns the storing of the new credentials.
+			logEvent(ctx, "hash conversion: computing a bcrypt hash with cost %d (from SCRAM iteration count %d)", bcryptCost, scramIters)
+		}
+
+		newHash, err := hashAndValidate(ctx, bcryptCost, HashBCrypt, cleartext, hashSem)
+		if err != nil {
+			// This call only fail with hard errors.
+			return false, nil, nil, "", err
+		}
+		return true, sh.bytes, newHash, HashBCrypt.String(), nil
 	}
 
-	rawHash, err := hashPasswordUsingSCRAM(ctx, int(scramIterCount), cleartext, hashSem)
+	// Nothing to do.
+	return false, nil, nil, "", nil
+}
+
+// hashAndValidate hashes the cleartext password and validates that it can
+// be decoded.
+func hashAndValidate(
+	ctx context.Context, cost int, newHashMethod HashMethod, cleartext string, hashSem HashSemaphore,
+) (newHashBytes []byte, err error) {
+	rawHash, err := HashPassword(ctx, cost, newHashMethod, cleartext, hashSem)
 	if err != nil {
 		// This call only fail with hard errors.
-		return false, nil, nil, "", err
+		return nil, err
 	}
 
 	// Check the raw hash can be decoded using our decoder.
 	// This checks that we're able to re-parse what we just generated,
 	// which is a safety mechanism to ensure the result is valid.
-	expectedMethod := HashSCRAMSHA256
 	newHash := LoadPasswordHash(ctx, rawHash)
-	if newHash.Method() != expectedMethod {
+	if newHash.Method() != newHashMethod {
 		// In contrast to logs, we don't want the details of the hash in the error with %+v.
-		return false, nil, nil, "", errors.AssertionFailedf("programming error: re-hash failed to produce SCRAM hash, produced %T instead", newHash)
+		return nil, errors.AssertionFailedf("programming error: re-hash failed to produce %s hash, produced %T instead", newHashMethod, newHash)
 	}
-	return true, []byte(bh), rawHash, expectedMethod.String(), nil
+	return rawHash, nil
 }