Merge #74301

74301: sql,server: support SCRAM authentication for SQL sessions r=rafiss,JeffSwenson,bdarnell,aaron-crl,kpatron-cockroachlabs a=knz

Fixes #42519.
Fixes #74511.
Informs #65117.
Epic CRDB-5349

**tldr:** this PR adds support for the SCRAM-SHA-256 authentication method, as defined by IETF RFCs [5802](https://datatracker.ietf.org/doc/html/rfc5802) and [7677](https://datatracker.ietf.org/doc/html/rfc7677) and as supported by PostgreSQL. This offers [multiple security benefits](#scram-benefits) over CockroachDB's current use of cleartext password authentication. To use this feature,
1) the stored password hashes must use the SCRAM encoding (this requires a migration away from crdb's bcrypt-based hashes); and
2) one of the SCRAM authentication methods must be enabled explictly via the HBA configuration (cluster setting `server.host_based_authentication.configuration`).

### How to review this work

The addition of the feature goes as follows:

1. adding HBA syntax and authentication method hooks for `scram-sha-256` and `cert-scram-sha-256`. These are gated behind a new cluster version, so that previous-version nodes do not get confused by the new syntax.

   Split into separate PR: #74842

2. extending ALTER/CREATE USER/ROLE WITH PASSWORD to recognize SCRAM hashes. This allows operators to use these SQL statements to populate SCRAM hashes.
    (This should also be seen as the more desirable way to configure SQL user accounts: it ensures that the CockroachDB node never ever sees the cleartext password of an account. Even if we later support computing the SCRAM hash server-side, pre-computing the hash client-side should be seen and documented as the superior approach.)

   Split into separate PR: #74843

3. extending the existing cleartext methods inside CockroachDB (used for the HTTP interface and the `password`-based methods in SQL) to compare a cleartext password to a pre-computed SCRAM hash. This ensures that the existing password mechanisms that are cleartext-based continue to work even after the stored credentials start using the SCRAM encoding.

   Split into separate PRs: #74844 and #74845

4. extending the code created at step 1 to actually use the stored SCRAM credentials for client authentication. This achieves the main goal.

   Split into separate PRs: #74846 and #74847

5. making the hash method used to encode cleartext passwords passed to ALTER/CREATE USER/ROLE WITH PASSWORD configurable, using a new cluster setting `server.user_login.password_encryption`, and exposing its value via the pg-compatible session variable `password_encryption`.
   This is a convenience feature - properly secured deployments should not need this, as they should use pre-hashing client-side instead (see note at point 2 above).

   Split into separate PR: #74848

6. making crdb auto-select SCRAM authn if the stored password uses SCRAM. This is one step to ensuring that previous-version clusters are automatically opted into SCRAM.

   Split into separate PR: #74849

7. Auto-upgrade the stored credentials from bcrypt to SCRAM, when enabled and possible.

   Split into separate PR: #74850

The reviewer is invited to consider each PR in turn, read its commit message(s) to understand the goal of that PR, then review the PR to ascertain that the implementation (and relevant tests) match the goals announced in the commit message.


Co-authored-by: Raphael 'kena' Poss <knz@thaumogen.net>

docs/generated/eventlog.md

@@ -2278,6 +2278,26 @@ An event of type `drop_role` is recorded when a role is dropped.
 | `ApplicationName` | The application name for the session where the event was emitted. This is included in the event to ease filtering of logging output by application. Application names starting with a dollar sign (`$`) are not considered sensitive. | depends |
 | `PlaceholderValues` | The mapping of SQL placeholders to their values, for prepared statements. | yes |
 
+### `password_hash_converted`
+
+An event of type `password_hash_converted` is recorded when the password credentials
+are automatically converted server-side.
+
+
+| Field | Description | Sensitive |
+|--|--|--|
+| `RoleName` | The name of the user/role whose credentials have been converted. | yes |
+| `OldMethod` | The previous hash method. | no |
+| `NewMethod` | The new hash method. | no |
+
+
+#### Common fields
+
+| Field | Description | Sensitive |
+|--|--|--|
+| `Timestamp` | The timestamp of the event. Expressed as nanoseconds since the Unix epoch. | no |
+| `EventType` | The type of the event. | no |
+
 ## Telemetry events
 
 

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

@@ -68,10 +68,14 @@ 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.cert_password_method.auto_scram_promotion.enabled	boolean	true	whether to automatically promote cert-password authentication to use SCRAM
 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.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.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.web_session.auto_logout.timeout	duration	168h0m0s	the duration that web sessions will survive before being periodically purged, since they were last used
 server.web_session.purge.max_deletions_per_cycle	integer	10	the maximum number of old sessions to delete for each purge
 server.web_session.purge.period	duration	1h0m0s	the time until old sessions are deleted
@@ -170,4 +174,4 @@ trace.debug.enable	boolean	false	if set, traces for recent requests can be seen
 trace.jaeger.agent	string		the address of a Jaeger agent to receive traces using the Jaeger UDP Thrift protocol, as <host>:<port>. If no port is specified, 6381 will be used.
 trace.opentelemetry.collector	string		address of an OpenTelemetry trace collector to receive traces using the otel gRPC protocol, as <host>:<port>. If no port is specified, 4317 will be used.
 trace.zipkin.collector	string		the address of a Zipkin instance to receive traces, as <host>:<port>. If no port is specified, 9411 will be used.
-version	version	21.2-44	set the active cluster version in the format '<major>.<minor>'
+version	version	21.2-46	set the active cluster version in the format '<major>.<minor>'

docs/generated/settings/settings.html

@@ -75,10 +75,14 @@
 <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.cert_password_method.auto_scram_promotion.enabled</code></td><td>boolean</td><td><code>true</code></td><td>whether to automatically promote cert-password authentication to use SCRAM</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_encryption</code></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><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.password_hashes.default_cost.scram_sha_256</code></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><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.user_login.upgrade_bcrypt_stored_passwords_to_scram.enabled</code></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><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>
 <tr><td><code>server.web_session.purge.max_deletions_per_cycle</code></td><td>integer</td><td><code>10</code></td><td>the maximum number of old sessions to delete for each purge</td></tr>
 <tr><td><code>server.web_session.purge.period</code></td><td>duration</td><td><code>1h0m0s</code></td><td>the time until old sessions are deleted</td></tr>
@@ -177,6 +181,6 @@
 <tr><td><code>trace.jaeger.agent</code></td><td>string</td><td><code></code></td><td>the address of a Jaeger agent to receive traces using the Jaeger UDP Thrift protocol, as <host>:<port>. If no port is specified, 6381 will be used.</td></tr>
 <tr><td><code>trace.opentelemetry.collector</code></td><td>string</td><td><code></code></td><td>address of an OpenTelemetry trace collector to receive traces using the otel gRPC protocol, as <host>:<port>. If no port is specified, 4317 will be used.</td></tr>
 <tr><td><code>trace.zipkin.collector</code></td><td>string</td><td><code></code></td><td>the address of a Zipkin instance to receive traces, as <host>:<port>. If no port is specified, 9411 will be used.</td></tr>
-<tr><td><code>version</code></td><td>version</td><td><code>21.2-44</code></td><td>set the active cluster version in the format '<major>.<minor>'</td></tr>
+<tr><td><code>version</code></td><td>version</td><td><code>21.2-46</code></td><td>set the active cluster version in the format '<major>.<minor>'</td></tr>
 </tbody>
 </table>

pkg/ccl/gssapiccl/BUILD.bazel

@@ -22,6 +22,7 @@ go_library(
         "@io_bazel_rules_go//go/platform:linux_amd64": [
             "//pkg/ccl/utilccl",
             "//pkg/security",
+            "//pkg/settings",
             "//pkg/sql",
             "//pkg/sql/sem/tree",
             "//pkg/sql/pgwire",

pkg/ccl/gssapiccl/gssapi.go

@@ -22,6 +22,7 @@ import (
 
 	"github.com/cockroachdb/cockroach/pkg/ccl/utilccl"
 	"github.com/cockroachdb/cockroach/pkg/security"
+	"github.com/cockroachdb/cockroach/pkg/settings"
 	"github.com/cockroachdb/cockroach/pkg/sql"
 	"github.com/cockroachdb/cockroach/pkg/sql/pgwire"
 	"github.com/cockroachdb/cockroach/pkg/sql/pgwire/hba"
@@ -113,7 +114,7 @@ func authGSS(
 
 // checkEntry validates that the HBA entry contains exactly one of the
 // include_realm=0 directive or an identity-mapping configuration.
-func checkEntry(entry hba.Entry) error {
+func checkEntry(_ *settings.Values, entry hba.Entry) error {
 	hasInclude0 := false
 	hasMap := false
 	for _, op := range entry.Options {

pkg/ccl/serverccl/role_authentication_test.go

@@ -129,7 +129,7 @@ func TestVerifyPassword(t *testing.T) {
 		t.Run(tc.testName, func(t *testing.T) {
 			execCfg := s.ExecutorConfig().(sql.ExecutorConfig)
 			username := security.MakeSQLUsernameFromPreNormalizedString(tc.username)
-			exists, canLoginSQL, canLoginDBConsole, isSuperuser, validUntil, _, pwRetrieveFn, err := sql.GetUserSessionInitInfo(
+			exists, canLoginSQL, canLoginDBConsole, isSuperuser, _, pwRetrieveFn, err := sql.GetUserSessionInitInfo(
 				context.Background(), &execCfg, &ie, username, "", /* databaseName */
 			)
 
@@ -153,26 +153,27 @@ func TestVerifyPassword(t *testing.T) {
 			if !exists || !canLoginDBConsole {
 				validDBConsole = false
 			}
+			if exists && (canLoginSQL || canLoginDBConsole) {
+				var hashedPassword security.PasswordHash
+				expired, hashedPassword, err = pwRetrieveFn(ctx)
+				if err != nil {
+					t.Errorf(
+						"credentials %s/%s failed with error %s, wanted no error",
+						tc.username,
+						tc.password,
+						err,
+					)
+				}
 
-			hashedPassword, err := pwRetrieveFn(ctx)
-			if err != nil {
-				t.Errorf(
-					"credentials %s/%s failed with error %s, wanted no error",
-					tc.username,
-					tc.password,
-					err,
-				)
-			}
-
-			err = security.CompareHashAndPassword(ctx, hashedPassword, tc.password)
-			if err != nil {
-				valid = false
-				validDBConsole = false
-			}
-
-			if validUntil != nil {
-				if validUntil.Time.Sub(timeutil.Now()) < 0 {
-					expired = true
+				pwCompare, err := security.CompareHashAndCleartextPassword(ctx, hashedPassword, tc.password)
+				if err != nil {
+					t.Error(err)
+					valid = false
+					validDBConsole = false
+				}
+				if !pwCompare {
+					valid = false
+					validDBConsole = false
 				}
 			}
 
@@ -190,7 +191,7 @@ func TestVerifyPassword(t *testing.T) {
 					"db console credentials %s/%s valid = %t, wanted %t",
 					tc.username,
 					tc.password,
-					valid,
+					validDBConsole,
 					tc.shouldAuthenticateDBConsole,
 				)
 			}

pkg/ccl/sqlproxyccl/authentication.go

@@ -12,7 +12,7 @@ import (
 	"net"
 
 	"github.com/cockroachdb/cockroach/pkg/ccl/sqlproxyccl/throttler"
-	"github.com/jackc/pgproto3/v2"
+	pgproto3 "github.com/jackc/pgproto3/v2"
 )
 
 // authenticate handles the startup of the pgwire protocol to the point where
@@ -49,10 +49,27 @@ var authenticate = func(clientConn, crdbConn net.Conn, throttleHook func(throttl
 		case
 			*pgproto3.AuthenticationCleartextPassword,
 			*pgproto3.AuthenticationMD5Password,
+			*pgproto3.AuthenticationSASLContinue,
+			*pgproto3.AuthenticationSASLFinal,
 			*pgproto3.AuthenticationSASL:
 			if err = feSend(backendMsg); err != nil {
 				return err
 			}
+			switch backendMsg.(type) {
+			case *pgproto3.AuthenticationCleartextPassword:
+				_ = fe.SetAuthType(pgproto3.AuthTypeCleartextPassword)
+			case *pgproto3.AuthenticationMD5Password:
+				_ = fe.SetAuthType(pgproto3.AuthTypeMD5Password)
+			case *pgproto3.AuthenticationSASLContinue:
+				_ = fe.SetAuthType(pgproto3.AuthTypeSASLContinue)
+			case *pgproto3.AuthenticationSASL:
+				_ = fe.SetAuthType(pgproto3.AuthTypeSASL)
+			case *pgproto3.AuthenticationSASLFinal:
+				// Final SCRAM message. Nothing more to expect from the
+				// client: the next message will be from the server and be
+				// AuthenticationOk or AuthenticationFail.
+				continue
+			}
 			fntMsg, err := fe.Receive()
 			if err != nil {
 				return newErrorf(codeClientReadFailed, "unable to receive message from client: %v", err)

pkg/ccl/sqlproxyccl/proxy_handler_test.go

@@ -225,10 +225,10 @@ func TestProxyAgainstSecureCRDB(t *testing.T) {
 	)
 
 	url := fmt.Sprintf("postgres://bob:wrong@%s/dim-dog-28.defaultdb?sslmode=require", addr)
-	te.TestConnectErr(ctx, t, url, 0, "ERROR: password authentication failed for user bob")
+	te.TestConnectErr(ctx, t, url, 0, "failed SASL auth")
 
 	url = fmt.Sprintf("postgres://bob@%s/dim-dog-28.defaultdb?sslmode=require", addr)
-	te.TestConnectErr(ctx, t, url, 0, "ERROR: password authentication failed for user bob")
+	te.TestConnectErr(ctx, t, url, 0, "failed SASL auth")
 
 	url = fmt.Sprintf("postgres://bob:builder@%s/dim-dog-28.defaultdb?sslmode=require", addr)
 	te.TestConnect(ctx, t, url, func(conn *pgx.Conn) {
@@ -449,7 +449,7 @@ func TestInsecureProxy(t *testing.T) {
 	)
 
 	url := fmt.Sprintf("postgres://bob:wrong@%s?sslmode=disable&options=--cluster=dim-dog-28&sslmode=require", addr)
-	te.TestConnectErr(ctx, t, url, 0, "ERROR: password authentication failed for user bob")
+	te.TestConnectErr(ctx, t, url, 0, "failed SASL auth")
 
 	url = fmt.Sprintf("postgres://bob:builder@%s/?sslmode=disable&options=--cluster=dim-dog-28&sslmode=require", addr)
 	te.TestConnect(ctx, t, url, func(conn *pgx.Conn) {

pkg/clusterversion/cockroach_versions.go

@@ -242,6 +242,8 @@ const (
 	// ScanWholeRows is the version at which the Header.WholeRowsOfSize parameter
 	// was introduced, preventing limited scans from returning partial rows.
 	ScanWholeRows
+	// SCRAM authentication is available.
+	SCRAMAuthentication
 
 	// *************************************************
 	// Step (1): Add new versions here.
@@ -366,6 +368,10 @@ var versionsSingleton = keyedVersions{
 		Key:     ScanWholeRows,
 		Version: roachpb.Version{Major: 21, Minor: 2, Internal: 44},
 	},
+	{
+		Key:     SCRAMAuthentication,
+		Version: roachpb.Version{Major: 21, Minor: 2, Internal: 46},
+	},
 
 	// *************************************************
 	// Step (2): Add new versions here.

pkg/security/BUILD.bazel

@@ -22,6 +22,7 @@ go_library(
     importpath = "github.com/cockroachdb/cockroach/pkg/security",
     visibility = ["//visibility:public"],
     deps = [
+        "//pkg/clusterversion",
         "//pkg/server/telemetry",
         "//pkg/settings",
         "//pkg/settings/cluster",
@@ -42,9 +43,11 @@ go_library(
         "@com_github_cockroachdb_errors//:errors",
         "@com_github_cockroachdb_errors//oserror",
         "@com_github_cockroachdb_redact//:redact",
+        "@com_github_xdg_go_scram//:scram",
         "@com_github_xdg_go_stringprep//:stringprep",
         "@org_golang_x_crypto//bcrypt",
         "@org_golang_x_crypto//ocsp",
+        "@org_golang_x_crypto//pbkdf2",
         "@org_golang_x_sync//errgroup",
     ],
 )
@@ -62,6 +65,7 @@ go_test(
         "certs_test.go",
         "join_token_test.go",
         "main_test.go",
+        "password_test.go",
         "permission_check_test.go",
         "tls_test.go",
         "username_test.go",
@@ -74,6 +78,7 @@ go_test(
         "//pkg/rpc",
         "//pkg/security/securitytest",
         "//pkg/server",
+        "//pkg/settings/cluster",
         "//pkg/testutils",
         "//pkg/testutils/serverutils",
         "//pkg/util/envutil",
@@ -84,6 +89,7 @@ go_test(
         "//pkg/util/uuid",
         "@com_github_cockroachdb_errors//:errors",
         "@com_github_stretchr_testify//require",
+        "@org_golang_x_crypto//bcrypt",
         "@org_golang_x_exp//rand",
     ] + select({
         "@io_bazel_rules_go//go/platform:aix": [

pkg/security/auth.go

@@ -157,7 +157,9 @@ func IsTenantCertificate(cert *x509.Certificate) bool {
 
 // UserAuthPasswordHook builds an authentication hook based on the security
 // mode, password, and its potentially matching hash.
-func UserAuthPasswordHook(insecureMode bool, password string, hashedPassword []byte) UserAuthHook {
+func UserAuthPasswordHook(
+	insecureMode bool, password string, hashedPassword PasswordHash,
+) UserAuthHook {
 	return func(ctx context.Context, systemIdentity SQLUsername, clientConnection bool) error {
 		if systemIdentity.Undefined() {
 			return errors.New("user is missing")
@@ -172,15 +174,24 @@ func UserAuthPasswordHook(insecureMode bool, password string, hashedPassword []b
 		}
 
 		// If the requested user has an empty password, disallow authentication.
-		if len(password) == 0 || CompareHashAndPassword(ctx, hashedPassword, password) != nil {
-			return errors.Errorf(ErrPasswordUserAuthFailed, systemIdentity)
+		if len(password) == 0 {
+			return NewErrPasswordUserAuthFailed(systemIdentity)
+		}
+		ok, err := CompareHashAndCleartextPassword(ctx, hashedPassword, password)
+		if err != nil {
+			return err
+		}
+		if !ok {
+			return NewErrPasswordUserAuthFailed(systemIdentity)
 		}
 
 		return nil
 	}
 }
 
-// ErrPasswordUserAuthFailed is the error template for failed password auth
-// of a user. It should be used when the password is incorrect or the user
-// does not exist.
-const ErrPasswordUserAuthFailed = "password authentication failed for user %s"
+// NewErrPasswordUserAuthFailed constructs an error that represents
+// failed password authentication for a user. It should be used when
+// the password is incorrect or the user does not exist.
+func NewErrPasswordUserAuthFailed(username SQLUsername) error {
+	return errors.Newf("password authentication failed for user %s", username)
+}