sql: properly handle NULL hashedPassword column in system.users

pkg/kv/kvserver/protectedts/ptstorage/sql.go

@@ -59,7 +59,7 @@ FROM (
         version + 1 AS new_version,
         num_records + 1 AS new_num_records, 
         num_spans + $3 AS new_num_spans, 
-        total_bytes + length($9) + length($6) + length($7) AS new_total_bytes
+        total_bytes + length($9) + length($6) + coalesce(length($7:::BYTES),0) AS new_total_bytes
     FROM
         current_meta
 )
@@ -135,7 +135,7 @@ RETURNING
 SELECT
     id,
     num_spans AS record_spans,
-    length(spans) + length(meta_type) + length(meta) AS record_bytes
+    length(spans) + length(meta_type) + coalesce(length(meta),0) AS record_bytes
 FROM
     system.protected_ts_records
 WHERE

pkg/kv/kvserver/protectedts/ptstorage/storage.go

@@ -67,13 +67,22 @@ func (p *storage) Protect(ctx context.Context, txn *kv.Txn, r *ptpb.Record) erro
 	if err != nil { // how can this possibly fail?
 		return errors.Wrap(err, "failed to marshal spans")
 	}
+	meta := r.Meta
+	if meta == nil {
+		// v20.1 crashes in rowToRecord and storage.Release if it finds a NULL
+		// value in system.protected_ts_records.meta. v20.2 and above handle
+		// this correctly, but we need to maintain mixed version compatibility
+		// for at least one release.
+		// TODO(nvanbenschoten): remove this for v21.1.
+		meta = []byte{}
+	}
 	s := makeSettings(p.settings)
 	rows, err := p.ex.QueryEx(ctx, "protectedts-protect", txn,
 		sqlbase.InternalExecutorSessionDataOverride{User: security.NodeUser},
 		protectQuery,
 		s.maxSpans, s.maxBytes, len(r.Spans),
 		r.ID.GetBytesMut(), r.Timestamp.AsOfSystemTime(),
-		r.MetaType, r.Meta,
+		r.MetaType, meta,
 		len(r.Spans), encodedSpans)
 	if err != nil {
 		return errors.Wrapf(err, "failed to write record %v", r.ID)
@@ -218,8 +227,10 @@ func rowToRecord(ctx context.Context, row tree.Datums, r *ptpb.Record) error {
 	r.Timestamp = ts
 
 	r.MetaType = string(*row[2].(*tree.DString))
-	if meta := row[3].(*tree.DBytes); meta != nil && len(*meta) > 0 {
-		r.Meta = []byte(*meta)
+	if row[3] != tree.DNull {
+		if meta := row[3].(*tree.DBytes); len(*meta) > 0 {
+			r.Meta = []byte(*meta)
+		}
 	}
 	var spans Spans
 	if err := protoutil.Unmarshal([]byte(*row[4].(*tree.DBytes)), &spans); err != nil {

pkg/sql/alter_role.go

@@ -146,6 +146,15 @@ func (n *alterRoleNode) startExec(params runParams) error {
 				"setting or updating a password is not supported in insecure mode")
 		}
 
+		if hashedPassword == nil {
+			// v20.1 and below crash during authentication if they find a NULL value
+			// in system.users.hashedPassword. v20.2 and above handle this correctly,
+			// but we need to maintain mixed version compatibility for at least one
+			// release.
+			// TODO(nvanbenschoten): remove this for v21.1.
+			hashedPassword = []byte{}
+		}
+
 		// Updating PASSWORD is a special case since PASSWORD lives in system.users
 		// while the rest of the role options lives in system.role_options.
 		_, err = params.extendedEvalCtx.ExecCfg.InternalExecutor.Exec(

pkg/sql/create_role.go

@@ -125,6 +125,13 @@ func (n *CreateRoleNode) startExec(params runParams) error {
 			return pgerror.New(pgcode.InvalidPassword,
 				"setting or updating a password is not supported in insecure mode")
 		}
+	} else {
+		// v20.1 and below crash during authentication if they find a NULL value
+		// in system.users.hashedPassword. v20.2 and above handle this correctly,
+		// but we need to maintain mixed version compatibility for at least one
+		// release.
+		// TODO(nvanbenschoten): remove this for v21.1.
+		hashedPassword = []byte{}
 	}
 
 	// Reject the "public" role. It does not have an entry in the users table but is reserved.

pkg/sql/exec_util.go

@@ -905,8 +905,10 @@ func golangFillQueryArguments(args ...interface{}) (tree.Datums, error) {
 			case reflect.String:
 				d = tree.NewDString(val.String())
 			case reflect.Slice:
-				// Handle byte slices.
-				if val.Type().Elem().Kind() == reflect.Uint8 {
+				switch {
+				case val.IsNil():
+					d = tree.DNull
+				case val.Type().Elem().Kind() == reflect.Uint8:
 					d = tree.NewDBytes(tree.DBytes(val.Bytes()))
 				}
 			}

pkg/sql/pgwire/testdata/auth/conn_log

@@ -267,4 +267,3 @@ I: [n1,client=XXX,local] 81 disconnected; duration: XXX
 subtest end
 
 subtest end
-

pkg/sql/pgwire/testdata/auth/special_cases

@@ -93,3 +93,34 @@ DROP USER testuser; CREATE USER testuser
 ok
 
 subtest end user_has_both_cert_and_passwd
+
+subtest user_has_null_hashed_password_column
+
+# This test manually adds a user to the system.users table with a NULL (not
+# empty) hashedPassword and attempts to log in as that user. This used to crash
+# the server (and this test) because the authentication routine only properly
+# handled empty hashedPassword values. See #48769.
+
+sql
+INSERT INTO system.users (username, "hashedPassword") VALUES ('nopassword', NULL)
+----
+ok
+
+set_hba
+host all nopassword 0.0.0.0/0 password
+----
+# Active authentication configuration on this node:
+# Original configuration:
+# host  all root all cert-password # CockroachDB mandatory rule
+# host all nopassword 0.0.0.0/0 password
+#
+# Interpreted configuration:
+# TYPE DATABASE USER       ADDRESS   METHOD        OPTIONS
+host   all      root       all       cert-password
+host   all      nopassword 0.0.0.0/0 password
+
+connect user=nopassword
+----
+ERROR: password authentication failed for user nopassword
+
+subtest end user_has_null_hashed_password_column

pkg/sql/user.go

@@ -119,7 +119,9 @@ func retrieveUserAndPassword(
 		}
 		if values != nil {
 			exists = true
-			hashedPassword = []byte(*(values[0].(*tree.DBytes)))
+			if v := values[0]; v != tree.DNull {
+				hashedPassword = []byte(*(v.(*tree.DBytes)))
+			}
 		}
 
 		if !exists {

pkg/sql/values_test.go

@@ -182,56 +182,57 @@ func TestGolangQueryArgs(t *testing.T) {
 	// type
 	testCases := []struct {
 		value        interface{}
-		expectedType reflect.Type
+		expectedType *types.T
 	}{
 		// Null type.
-		{nil, reflect.TypeOf(types.Unknown)},
+		{nil, types.Unknown},
+		{[]byte(nil), types.Unknown},
 
 		// Bool type.
-		{true, reflect.TypeOf(types.Bool)},
+		{true, types.Bool},
 
 		// Primitive Integer types.
-		{int(1), reflect.TypeOf(types.Int)},
-		{int8(1), reflect.TypeOf(types.Int)},
-		{int16(1), reflect.TypeOf(types.Int)},
-		{int32(1), reflect.TypeOf(types.Int)},
-		{int64(1), reflect.TypeOf(types.Int)},
-		{uint(1), reflect.TypeOf(types.Int)},
-		{uint8(1), reflect.TypeOf(types.Int)},
-		{uint16(1), reflect.TypeOf(types.Int)},
-		{uint32(1), reflect.TypeOf(types.Int)},
-		{uint64(1), reflect.TypeOf(types.Int)},
+		{int(1), types.Int},
+		{int8(1), types.Int},
+		{int16(1), types.Int},
+		{int32(1), types.Int},
+		{int64(1), types.Int},
+		{uint(1), types.Int},
+		{uint8(1), types.Int},
+		{uint16(1), types.Int},
+		{uint32(1), types.Int},
+		{uint64(1), types.Int},
 
 		// Primitive Float types.
-		{float32(1.0), reflect.TypeOf(types.Float)},
-		{float64(1.0), reflect.TypeOf(types.Float)},
+		{float32(1.0), types.Float},
+		{float64(1.0), types.Float},
 
 		// Decimal type.
-		{apd.New(55, 1), reflect.TypeOf(types.Decimal)},
+		{apd.New(55, 1), types.Decimal},
 
 		// String type.
-		{"test", reflect.TypeOf(types.String)},
+		{"test", types.String},
 
 		// Bytes type.
-		{[]byte("abc"), reflect.TypeOf(types.Bytes)},
+		{[]byte("abc"), types.Bytes},
 
 		// Interval and timestamp.
-		{time.Duration(1), reflect.TypeOf(types.Interval)},
-		{timeutil.Now(), reflect.TypeOf(types.Timestamp)},
+		{time.Duration(1), types.Interval},
+		{timeutil.Now(), types.Timestamp},
 
 		// Primitive type aliases.
-		{roachpb.NodeID(1), reflect.TypeOf(types.Int)},
-		{sqlbase.ID(1), reflect.TypeOf(types.Int)},
-		{floatAlias(1), reflect.TypeOf(types.Float)},
-		{boolAlias(true), reflect.TypeOf(types.Bool)},
-		{stringAlias("string"), reflect.TypeOf(types.String)},
+		{roachpb.NodeID(1), types.Int},
+		{sqlbase.ID(1), types.Int},
+		{floatAlias(1), types.Float},
+		{boolAlias(true), types.Bool},
+		{stringAlias("string"), types.String},
 
 		// Byte slice aliases.
-		{roachpb.Key("key"), reflect.TypeOf(types.Bytes)},
-		{roachpb.RKey("key"), reflect.TypeOf(types.Bytes)},
+		{roachpb.Key("key"), types.Bytes},
+		{roachpb.RKey("key"), types.Bytes},
 
 		// Bit array.
-		{bitarray.MakeBitArrayFromInt64(8, 58, 7), reflect.TypeOf(types.VarBit)},
+		{bitarray.MakeBitArrayFromInt64(8, 58, 7), types.VarBit},
 	}
 
 	for i, tcase := range testCases {
@@ -241,7 +242,7 @@ func TestGolangQueryArgs(t *testing.T) {
 			t.Fatalf("expected 1 datum, got: %d", len(datums))
 		}
 		d := datums[0]
-		if a, e := reflect.TypeOf(d.ResolvedType()), tcase.expectedType; a != e {
+		if a, e := d.ResolvedType(), tcase.expectedType; !a.Equal(e) {
 			t.Errorf("case %d failed: expected type %s, got %s", i, e.String(), a.String())
 		}
 	}