sql: prevent dropped user from accessing the cluster

Release note (bug fix): Previously if a user was logged in while a
different session dropped that user, the dropped user would still
inherit privileges from the `public` role. Now, CockroachDB checks that
the user exists before allowing it to inherit privileges from the
`public` role.

In addition, any active web sessions are now revoked when a user is dropped.

pkg/bench/rttanalysis/testdata/benchmark_expectations

@@ -40,8 +40,8 @@ exp,benchmark
 17,DropDatabase/drop_database_2_tables
 17,DropDatabase/drop_database_3_tables
 26,DropRole/drop_1_role
-34,DropRole/drop_2_roles
-42,DropRole/drop_3_roles
+36,DropRole/drop_2_roles
+45,DropRole/drop_3_roles
 15,DropSequence/drop_1_sequence
 17,DropSequence/drop_2_sequences
 19,DropSequence/drop_3_sequences

pkg/ccl/backupccl/backuptestutils/testutils.go

@@ -187,6 +187,11 @@ func CheckForInvalidDescriptors(t testing.TB, sqlDB *gosql.DB) {
 	// acquisition and allows the query to fetch all descriptors in the cluster.
 	rows, err := sqlDB.Query(`SELECT id, obj_name, error FROM "".crdb_internal.invalid_objects`)
 	if err != nil {
+		if testutils.IsError(err, "role .* was concurrently dropped") {
+			// Some tests do not restore users, so the user who owned this session may
+			// no longer exist.
+			return
+		}
 		t.Fatal(err)
 	}
 	invalidIDs, err := sqlutils.RowsToDataDrivenOutput(rows)

pkg/sql/authorization.go

@@ -175,6 +175,21 @@ func (p *planner) HasPrivilege(
 
 	// Check if the 'public' pseudo-role has privileges.
 	if privs.CheckPrivilege(username.PublicRoleName(), privilegeKind) {
+		// Before returning true, make sure the user actually exists.
+		// We only need to check for existence here, since a dropped user will not
+		// appear in the role hierarchy, so it cannot pass the privilege check made
+		// later in this function. The RoleExists call performs a system table
+		// lookup, so it's better not to do it in the general case.
+		if user.IsNodeUser() || user.IsRootUser() {
+			// Short-circuit for the node and root users to avoid doing an extra
+			// lookup in common cases (e.g., internal executor usages).
+			return true, nil
+		}
+		if exists, err := p.RoleExists(ctx, user); err != nil {
+			return false, err
+		} else if !exists {
+			return false, pgerror.Newf(pgcode.UndefinedObject, "role %s was concurrently dropped", user)
+		}
 		return true, nil
 	}
 

pkg/sql/drop_role.go

@@ -451,6 +451,17 @@ func (n *DropRoleNode) startExec(params runParams) error {
 		}
 		numRoleSettingsRowsDeleted += rowsDeleted
 
+		_, err = params.p.InternalSQLTxn().ExecEx(
+			params.ctx,
+			opName,
+			params.p.txn,
+			sessiondata.NodeUserSessionDataOverride,
+			`UPDATE system.web_sessions SET "revokedAt" = now() WHERE username = $1 AND "revokedAt" IS NULL;`,
+			normalizedUsername,
+		)
+		if err != nil {
+			return err
+		}
 	}
 
 	// Bump role-related table versions to force a refresh of membership/auth

pkg/sql/tests/BUILD.bazel

@@ -37,6 +37,7 @@ go_test(
         "autocommit_extended_protocol_test.go",
         "bank_test.go",
         "copy_file_upload_test.go",
+        "drop_role_concurrent_session_test.go",
         "empty_query_test.go",
         "enum_test.go",
         "hash_sharded_test.go",

pkg/sql/tests/drop_role_concurrent_session_test.go

@@ -0,0 +1,98 @@
+// Copyright 2023 The Cockroach Authors.
+//
+// Use of this software is governed by the Business Source License
+// included in the file licenses/BSL.txt.
+//
+// As of the Change Date specified in that file, in accordance with
+// the Business Source License, use of this software will be governed
+// by the Apache License, Version 2.0, included in the file
+// licenses/APL.txt.
+
+package tests
+
+import (
+	"context"
+	"net/url"
+	"testing"
+	"time"
+
+	"github.com/cockroachdb/cockroach/pkg/base"
+	"github.com/cockroachdb/cockroach/pkg/testutils/serverutils"
+	"github.com/cockroachdb/cockroach/pkg/testutils/sqlutils"
+	"github.com/cockroachdb/cockroach/pkg/util/leaktest"
+	"github.com/cockroachdb/cockroach/pkg/util/log"
+	"github.com/cockroachdb/cockroach/pkg/util/timeutil"
+	"github.com/jackc/pgx/v4"
+	"github.com/stretchr/testify/require"
+)
+
+// Test that if a user is dropped while a session is active, the session can
+// no longer access objects in the cluster.
+func TestDropRoleConcurrentSession(t *testing.T) {
+	defer leaktest.AfterTest(t)()
+	defer log.Scope(t).Close(t)
+
+	ctx := context.Background()
+	s, rootDB, _ := serverutils.StartServer(t, base.TestServerArgs{})
+	defer s.Stopper().Stop(ctx)
+
+	_, err := rootDB.Exec(`CREATE USER testuser`)
+	require.NoError(t, err)
+	_, err = rootDB.Exec(`CREATE ROLE testrole`)
+	require.NoError(t, err)
+	_, err = rootDB.Exec(`GRANT testrole TO testuser`)
+	require.NoError(t, err)
+	_, err = rootDB.Exec(`CREATE TABLE inherit_from_public (a INT PRIMARY KEY)`)
+	require.NoError(t, err)
+	_, err = rootDB.Exec(`INSERT INTO inherit_from_public VALUES (1)`)
+	require.NoError(t, err)
+	_, err = rootDB.Exec(`GRANT SELECT ON inherit_from_public TO public`)
+	require.NoError(t, err)
+	_, err = rootDB.Exec(`CREATE TABLE inherit_from_role (a INT PRIMARY KEY)`)
+	require.NoError(t, err)
+	_, err = rootDB.Exec(`INSERT INTO inherit_from_role VALUES (2)`)
+	require.NoError(t, err)
+	_, err = rootDB.Exec(`GRANT SELECT ON inherit_from_role TO testrole`)
+	require.NoError(t, err)
+
+	// Start a session as testuser.
+	testUserURL, cleanupFunc := sqlutils.PGUrl(t, s.ServingSQLAddr(), "TestDropRoleConcurrentSession", url.User("testuser"))
+	defer cleanupFunc()
+	testuserConn, err := pgx.Connect(ctx, testUserURL.String())
+	require.NoError(t, err)
+	defer func() { _ = testuserConn.Close(ctx) }()
+
+	// Also create a web session for testuser.
+	_, err = rootDB.Exec(`INSERT INTO system.web_sessions ("hashedSecret", username, "expiresAt", user_id)
+  VALUES ($1, $2, $3, (SELECT user_id FROM system.users WHERE username = $2))`,
+		"secret", "testuser", timeutil.Now().Add(24*time.Hour))
+	require.NoError(t, err)
+
+	// Verify that testuser can access the cluster.
+	var i int
+	err = testuserConn.QueryRow(ctx, `SELECT * FROM inherit_from_public`).Scan(&i)
+	require.NoError(t, err)
+	require.Equal(t, 1, i)
+	err = testuserConn.QueryRow(ctx, `SELECT * FROM inherit_from_role`).Scan(&i)
+	require.NoError(t, err)
+	require.Equal(t, 2, i)
+	_, err = testuserConn.Exec(ctx, "CREATE TABLE new1(a INT PRIMARY KEY)")
+	require.NoError(t, err)
+	_, err = testuserConn.Exec(ctx, "DROP TABLE new1")
+	require.NoError(t, err)
+
+	// Drop testuser, and verify that it no longer has access.
+	_, err = rootDB.Exec(`DROP USER testuser`)
+	require.NoError(t, err)
+	err = testuserConn.QueryRow(ctx, `SELECT * FROM inherit_from_public`).Scan(&i)
+	require.ErrorContains(t, err, "role testuser was concurrently dropped")
+	err = testuserConn.QueryRow(ctx, `SELECT * FROM inherit_from_role`).Scan(&i)
+	require.ErrorContains(t, err, "role testuser was concurrently dropped")
+	_, err = testuserConn.Exec(ctx, "CREATE TABLE new2(a INT PRIMARY KEY)")
+	require.ErrorContains(t, err, "role testuser was concurrently dropped")
+
+	var revokedAt time.Time
+	err = rootDB.QueryRow(`SELECT "revokedAt" FROM system.web_sessions WHERE username = $1`, "testuser").Scan(&revokedAt)
+	require.NoError(t, err)
+	require.Less(t, revokedAt, timeutil.Now())
+}