From 12ec26ff28294a8c9e0f89b3dbcc2c60910fd8e5 Mon Sep 17 00:00:00 2001 From: Rafi Shamim Date: Thu, 1 Jun 2023 13:00:35 -0400 Subject: [PATCH] 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. --- .../testdata/benchmark_expectations | 4 +- .../backupccl/backuptestutils/testutils.go | 5 + pkg/sql/authorization.go | 15 +++ pkg/sql/drop_role.go | 11 +++ pkg/sql/tests/BUILD.bazel | 1 + .../drop_role_concurrent_session_test.go | 98 +++++++++++++++++++ 6 files changed, 132 insertions(+), 2 deletions(-) create mode 100644 pkg/sql/tests/drop_role_concurrent_session_test.go diff --git a/pkg/bench/rttanalysis/testdata/benchmark_expectations b/pkg/bench/rttanalysis/testdata/benchmark_expectations index d71fce0faed9..323c5fff2c8e 100644 --- a/pkg/bench/rttanalysis/testdata/benchmark_expectations +++ b/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 diff --git a/pkg/ccl/backupccl/backuptestutils/testutils.go b/pkg/ccl/backupccl/backuptestutils/testutils.go index 21098b54716f..d56e6bceb86c 100644 --- a/pkg/ccl/backupccl/backuptestutils/testutils.go +++ b/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) diff --git a/pkg/sql/authorization.go b/pkg/sql/authorization.go index e377c48b029b..63fab9ca90c2 100644 --- a/pkg/sql/authorization.go +++ b/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 } diff --git a/pkg/sql/drop_role.go b/pkg/sql/drop_role.go index e9a297ebdcf5..9052aab021bb 100644 --- a/pkg/sql/drop_role.go +++ b/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 diff --git a/pkg/sql/tests/BUILD.bazel b/pkg/sql/tests/BUILD.bazel index c949b16c0d58..0d17cd762320 100644 --- a/pkg/sql/tests/BUILD.bazel +++ b/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", diff --git a/pkg/sql/tests/drop_role_concurrent_session_test.go b/pkg/sql/tests/drop_role_concurrent_session_test.go new file mode 100644 index 000000000000..50d7d86548a1 --- /dev/null +++ b/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()) +}