privilege,externalconn: add CREATEEXTERNALCONNECTION system privilege

This change introduces a `CREATEEXTERNALCONNECTION` system privilege. This privilege is required by a user to be able to create an external connection. Root and admin have these system privileges by default, and are capable of granting this privilege to other users/roles with or without the grant option. Fixes: cockroachdb#85006 Release note (sql change): introduce a `CREATEEXTERNALCONNECTION` system privilege that is required to create an External Connection object to represent an underlying resource.
adityamaru · Jul 25, 2022 · 976fe96 · 976fe96
1 parent 48ffa80
commit 976fe96
Show file tree

Hide file tree

Showing 10 changed files with 153 additions and 48 deletions.
diff --git a/pkg/ccl/cloudccl/externalconn/datadriven_test.go b/pkg/ccl/cloudccl/externalconn/datadriven_test.go
@@ -63,6 +63,12 @@ func TestDataDriven(t *testing.T) {
 				externalConnTestCluster.InitializeTenant(ctx, tenantID)
 
 			case "exec-sql":
+				if d.HasArg("user") {
+					var user string
+					d.ScanArgs(t, "user", &user)
+					resetToRootUser := externalConnTestCluster.SetSQLDBForUser(tenantID, user)
+					defer resetToRootUser()
+				}
 				if err := tenant.ExecWithErr(d.Input); err != nil {
 					return fmt.Sprint(err.Error())
 				}

diff --git a/pkg/ccl/cloudccl/externalconn/testdata/create_drop_external_connection b/pkg/ccl/cloudccl/externalconn/testdata/create_drop_external_connection
@@ -45,3 +45,37 @@ inspect-system-table
 ----
 
 subtest end
+
+subtest create-external-connection-global-privilege
+
+exec-sql
+CREATE USER testuser;
+----
+
+exec-sql user=testuser
+CREATE EXTERNAL CONNECTION privileged AS 'nodelocal://1/foo'
+----
+pq: only users with the CREATEEXTERNALCONNECTION system privilege are allowed to CREATE EXTERNAL CONNECTION
+
+exec-sql
+GRANT SYSTEM CREATEEXTERNALCONNECTION TO testuser;
+----
+
+exec-sql user=testuser
+CREATE EXTERNAL CONNECTION privileged AS 'nodelocal://1/foo'
+----
+
+inspect-system-table
+----
+privileged STORAGE {"nodelocal": {"cfg": {"nodeId": 1, "path": "/foo"}}}
+
+exec-sql
+REVOKE SYSTEM CREATEEXTERNALCONNECTION FROM testuser;
+----
+
+exec-sql user=testuser
+CREATE EXTERNAL CONNECTION privileged AS 'nodelocal://1/foo'
+----
+pq: only users with the CREATEEXTERNALCONNECTION system privilege are allowed to CREATE EXTERNAL CONNECTION
+
+subtest end
diff --git a/pkg/cloud/externalconn/record.go b/pkg/cloud/externalconn/record.go
@@ -183,9 +183,12 @@ func (e *ExternalConnection) Create(
 		return err
 	}
 
+	// CREATE EXTERNAL CONNECTION is only allowed for users with the
+	// `CREATEEXTERNALCONNECTION` system privilege. We run the query as `node`
+	// since the user might not have `INSERT` on the system table.
 	createQuery := "INSERT INTO system.external_connections (%s) VALUES (%s) RETURNING connection_name"
 	row, retCols, err := ex.QueryRowExWithCols(ctx, "ExternalConnection.Create", txn,
-		sessiondata.InternalExecutorOverride{User: user},
+		sessiondata.InternalExecutorOverride{User: username.NodeUserName()},
 		fmt.Sprintf(createQuery, strings.Join(cols, ","), generatePlaceholders(len(qargs))),
 		qargs...,
 	)

diff --git a/pkg/cloud/externalconn/utils/cluster.go b/pkg/cloud/externalconn/utils/cluster.go
@@ -42,15 +42,55 @@ func NewHandle(t *testing.T, tc *testcluster.TestCluster) *Handle {
 	}
 }
 
+// SetSQLDBForUser sets the tenants' SQL runner to a PGURL connection using the
+// passed in `user`. The method returns a function that resets the tenants' SQL
+// runner to a PGURL connection for the root user.
+func (h *Handle) SetSQLDBForUser(tenantID roachpb.TenantID, user string) func() {
+	tenantState, ok := h.ts[tenantID]
+	if !ok {
+		h.t.Fatalf("tenant ID %d has not been initialized", tenantID)
+	}
+
+	resetToRootUser := func() {
+		tenantState.curDB = tenantState.userToDB[username.RootUserName().Normalized()]
+	}
+
+	if runner, ok := tenantState.userToDB[user]; ok {
+		tenantState.curDB = runner
+		return resetToRootUser
+	}
+
+	pgURL, cleanup := sqlutils.PGUrl(h.t, h.tc.Server(0).ServingSQLAddr(),
+		"TestBackupRestoreDataDriven", url.User(user))
+	userSQLDB, err := gosql.Open("postgres", pgURL.String())
+	require.NoError(h.t, err)
+	tenantState.curDB = sqlutils.MakeSQLRunner(userSQLDB)
+	tenantState.userToDB[user] = tenantState.curDB
+	tenantState.cleanupFns = append(tenantState.cleanupFns, func(){
+		require.NoError(h.t, userSQLDB.Close())
+		cleanup()
+	})
+
+	return resetToRootUser
+}
+
 // InitializeTenant initializes a tenant with the given ID, returning the
 // relevant tenant state.
 func (h *Handle) InitializeTenant(ctx context.Context, tenID roachpb.TenantID) {
 	testServer := h.tc.Server(0)
-	tenantState := &Tenant{t: h.t}
+	tenantState := &Tenant{t: h.t, userToDB: make(map[string]*sqlutils.SQLRunner)}
 	if tenID == roachpb.SystemTenantID {
 		tenantState.TestTenantInterface = testServer
-		tenantState.db = sqlutils.MakeSQLRunner(h.tc.ServerConn(0))
-		tenantState.cleanup = func() {} // noop
+		pgURL, cleanupPGUrl := sqlutils.PGUrl(h.t, tenantState.SQLAddr(), "System",
+			url.User(username.RootUser))
+		userSQLDB, err := gosql.Open("postgres", pgURL.String())
+		require.NoError(h.t, err)
+		tenantState.curDB = sqlutils.MakeSQLRunner(userSQLDB)
+		tenantState.userToDB[username.RootUserName().Normalized()] = tenantState.curDB
+		tenantState.cleanupFns = append(tenantState.cleanupFns, func(){
+			require.NoError(h.t, userSQLDB.Close())
+			cleanupPGUrl()
+		})
 	} else {
 		tenantArgs := base.TestTenantArgs{
 			TenantID: tenID,
@@ -63,11 +103,12 @@ func (h *Handle) InitializeTenant(ctx context.Context, tenID roachpb.TenantID) {
 		tenantSQLDB, err := gosql.Open("postgres", pgURL.String())
 		require.NoError(h.t, err)
 
-		tenantState.db = sqlutils.MakeSQLRunner(tenantSQLDB)
-		tenantState.cleanup = func() {
+		tenantState.curDB = sqlutils.MakeSQLRunner(tenantSQLDB)
+		tenantState.userToDB[username.RootUserName().Normalized()] = tenantState.curDB
+		tenantState.cleanupFns = append(tenantState.cleanupFns, func(){
 			require.NoError(h.t, tenantSQLDB.Close())
 			cleanupPGUrl()
-		}
+		})
 	}
 
 	h.ts[tenID] = tenantState
@@ -91,7 +132,9 @@ func (h *Handle) Tenants() []*Tenant {
 // Cleanup frees up internal resources.
 func (h *Handle) Cleanup() {
 	for _, tenantState := range h.ts {
-		tenantState.cleanup()
+		for _, cleanup := range tenantState.cleanupFns {
+			cleanup()
+		}
 	}
 	h.ts = nil
 }
diff --git a/pkg/cloud/externalconn/utils/tenant_state.go b/pkg/cloud/externalconn/utils/tenant_state.go
@@ -24,30 +24,31 @@ import (
 type Tenant struct {
 	serverutils.TestTenantInterface
 
-	t       *testing.T
-	db      *sqlutils.SQLRunner
-	cleanup func()
+	t          *testing.T
+	userToDB   map[string]*sqlutils.SQLRunner
+	curDB      *sqlutils.SQLRunner
+	cleanupFns []func()
 }
 
 // Exec is a wrapper around gosql.Exec that kills the test on error. It records
 // the execution timestamp for subsequent use.
 func (s *Tenant) Exec(query string, args ...interface{}) {
-	s.db.Exec(s.t, query, args...)
+	s.curDB.Exec(s.t, query, args...)
 }
 
 // ExecWithErr is like Exec but returns the error, if any. It records the
 // execution timestamp for subsequent use.
 func (s *Tenant) ExecWithErr(query string, args ...interface{}) error {
-	_, err := s.db.DB.ExecContext(context.Background(), query, args...)
+	_, err := s.curDB.DB.ExecContext(context.Background(), query, args...)
 	return err
 }
 
 // Query is a wrapper around gosql.Query that kills the test on error.
 func (s *Tenant) Query(query string, args ...interface{}) *gosql.Rows {
-	return s.db.Query(s.t, query, args...)
+	return s.curDB.Query(s.t, query, args...)
 }
 
 // QueryWithErr is like Query but returns the error.
 func (s *Tenant) QueryWithErr(query string, args ...interface{}) (*gosql.Rows, error) {
-	return s.db.DB.QueryContext(context.Background(), query, args...)
+	return s.curDB.DB.QueryContext(context.Background(), query, args...)
 }
diff --git a/pkg/sql/create_external_connection.go b/pkg/sql/create_external_connection.go
@@ -17,7 +17,9 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/kv"
 	"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgcode"
 	"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror"
+	"github.com/cockroachdb/cockroach/pkg/sql/privilege"
 	"github.com/cockroachdb/cockroach/pkg/sql/sem/tree"
+	"github.com/cockroachdb/cockroach/pkg/sql/syntheticprivilege"
 	"github.com/cockroachdb/errors"
 )
 
@@ -60,16 +62,11 @@ func (p *planner) makeExternalConnectionEval(
 func (p *planner) createExternalConnection(
 	params runParams, n *tree.CreateExternalConnection,
 ) error {
-	// TODO(adityamaru): Check that the user has `CREATEEXTERNALCONNECTION` global
-	// privilege once we add support for it. Remove admin only check.
-	hasAdmin, err := params.p.HasAdminRole(params.ctx)
-	if err != nil {
-		return err
-	}
-	if !hasAdmin {
+	if err := params.p.CheckPrivilege(params.ctx, syntheticprivilege.GlobalPrivilegeObject,
+		privilege.CREATEEXTERNALCONNECTION); err != nil {
 		return pgerror.New(
 			pgcode.InsufficientPrivilege,
-			"only users with the admin role are allowed to CREATE EXTERNAL CONNECTION")
+			"only users with the CREATEEXTERNALCONNECTION system privilege are allowed to CREATE EXTERNAL CONNECTION")
 	}
 
 	// TODO(adityamaru): Add some metrics to track CREATE EXTERNAL CONNECTION

diff --git a/pkg/sql/logictest/BUILD.bazel b/pkg/sql/logictest/BUILD.bazel
@@ -16,6 +16,7 @@ go_library(
         "//pkg/base",
         "//pkg/build",
         "//pkg/build/bazel",
+        "//pkg/cloud/externalconn/providers",
         "//pkg/clusterversion",
         "//pkg/kv/kvserver",
         "//pkg/roachpb",

diff --git a/pkg/sql/logictest/logic.go b/pkg/sql/logictest/logic.go
@@ -41,6 +41,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/base"
 	"github.com/cockroachdb/cockroach/pkg/build"
 	"github.com/cockroachdb/cockroach/pkg/build/bazel"
+	_ "github.com/cockroachdb/cockroach/pkg/cloud/externalconn/providers" // imported to register ExternalConnection providers
 	"github.com/cockroachdb/cockroach/pkg/clusterversion"
 	"github.com/cockroachdb/cockroach/pkg/kv/kvserver"
 	"github.com/cockroachdb/cockroach/pkg/roachpb"

diff --git a/pkg/sql/logictest/testdata/logic_test/system_privileges b/pkg/sql/logictest/testdata/logic_test/system_privileges
@@ -3,26 +3,43 @@ user testuser
 statement error pq: user testuser does not have MODIFYCLUSTERSETTING privilege on Global
 SELECT * FROM crdb_internal.cluster_settings;
 
+statement error pq: only users with the CREATEEXTERNALCONNECTION system privilege are allowed to CREATE EXTERNAL CONNECTION
+CREATE EXTERNAL CONNECTION foo AS 'nodelocal://1/foo';
+
 user root
 
 statement ok
 GRANT SYSTEM MODIFYCLUSTERSETTING TO testuser
 
+statement ok
+GRANT SYSTEM CREATEEXTERNALCONNECTION TO testuser
+
 user testuser
 
 statement ok
 SELECT * FROM crdb_internal.cluster_settings;
 
+statement ok
+CREATE EXTERNAL CONNECTION foo AS 'nodelocal://1/foo';
+
 user root
 
 query TTTT
 SELECT * FROM system.privileges
 ----
-testuser  /global/  {MODIFYCLUSTERSETTING}  {}
+testuser  /global/  {CREATEEXTERNALCONNECTION,MODIFYCLUSTERSETTING}  {}
+
+query TT
+SELECT connection_name, connection_type FROM system.external_connections
+----
+foo  STORAGE
 
 statement ok
 REVOKE SYSTEM MODIFYCLUSTERSETTING FROM testuser
 
+statement ok
+REVOKE SYSTEM CREATEEXTERNALCONNECTION FROM testuser
+
 user testuser
 
 statement error pq: user testuser does not have MODIFYCLUSTERSETTING privilege on Global

diff --git a/pkg/sql/privilege/privilege.go b/pkg/sql/privilege/privilege.go
@@ -36,16 +36,17 @@ const (
 	DROP   Kind = 3
 	// DEPRECATEDGRANT is a placeholder to make sure that 4 is not reused.
 	// It was previously used for the GRANT privilege that has been replaced with the more granular Privilege.GrantOption.
-	DEPRECATEDGRANT      Kind = 4 // GRANT
-	SELECT               Kind = 5
-	INSERT               Kind = 6
-	DELETE               Kind = 7
-	UPDATE               Kind = 8
-	USAGE                Kind = 9
-	ZONECONFIG           Kind = 10
-	CONNECT              Kind = 11
-	RULE                 Kind = 12
-	MODIFYCLUSTERSETTING Kind = 13
+	DEPRECATEDGRANT          Kind = 4 // GRANT
+	SELECT                   Kind = 5
+	INSERT                   Kind = 6
+	DELETE                   Kind = 7
+	UPDATE                   Kind = 8
+	USAGE                    Kind = 9
+	ZONECONFIG               Kind = 10
+	CONNECT                  Kind = 11
+	RULE                     Kind = 12
+	MODIFYCLUSTERSETTING     Kind = 13
+	CREATEEXTERNALCONNECTION Kind = 14
 )
 
 // Privilege represents a privilege parsed from an Access Privilege Inquiry
@@ -102,7 +103,7 @@ var (
 	// certain privileges unavailable after upgrade migration.
 	// Note that "CREATE, INSERT, DELETE, ZONECONFIG" are no-op privileges on sequences.
 	SequencePrivileges = List{ALL, USAGE, SELECT, UPDATE, CREATE, DROP, INSERT, DELETE, ZONECONFIG}
-	SystemPrivileges   = List{ALL, MODIFYCLUSTERSETTING}
+	SystemPrivileges   = List{ALL, MODIFYCLUSTERSETTING, CREATEEXTERNALCONNECTION}
 )
 
 // Mask returns the bitmask for a given privilege.
@@ -117,23 +118,24 @@ func (k Kind) IsSetIn(bits uint32) bool {
 
 // ByValue is just an array of privilege kinds sorted by value.
 var ByValue = [...]Kind{
-	ALL, CREATE, DROP, SELECT, INSERT, DELETE, UPDATE, USAGE, ZONECONFIG, CONNECT, RULE, MODIFYCLUSTERSETTING,
+	ALL, CREATE, DROP, SELECT, INSERT, DELETE, UPDATE, USAGE, ZONECONFIG, CONNECT, RULE, MODIFYCLUSTERSETTING, CREATEEXTERNALCONNECTION,
 }
 
 // ByName is a map of string -> kind value.
 var ByName = map[string]Kind{
-	"ALL":                  ALL,
-	"CONNECT":              CONNECT,
-	"CREATE":               CREATE,
-	"DROP":                 DROP,
-	"SELECT":               SELECT,
-	"INSERT":               INSERT,
-	"DELETE":               DELETE,
-	"UPDATE":               UPDATE,
-	"ZONECONFIG":           ZONECONFIG,
-	"USAGE":                USAGE,
-	"RULE":                 RULE,
-	"MODIFYCLUSTERSETTING": MODIFYCLUSTERSETTING,
+	"ALL":                      ALL,
+	"CONNECT":                  CONNECT,
+	"CREATE":                   CREATE,
+	"DROP":                     DROP,
+	"SELECT":                   SELECT,
+	"INSERT":                   INSERT,
+	"DELETE":                   DELETE,
+	"UPDATE":                   UPDATE,
+	"ZONECONFIG":               ZONECONFIG,
+	"USAGE":                    USAGE,
+	"RULE":                     RULE,
+	"MODIFYCLUSTERSETTING":     MODIFYCLUSTERSETTING,
+	"CREATEEXTERNALCONNECTION": CREATEEXTERNALCONNECTION,
 }
 
 // List is a list of privileges.