diff --git a/pkg/ccl/cloudccl/externalconn/datadriven_test.go b/pkg/ccl/cloudccl/externalconn/datadriven_test.go index 6c54d4a21cb6..2de372a1e79c 100644 --- 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 index 25d054bbdc40..e187d16f3ac8 100644 --- 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 index 02b7f4a54d93..cc3f496ac919 100644 --- 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 index d3a48c885a0b..9720d7130d08 100644 --- 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 index 2a5c6fc4ddf6..bb47cc348667 100644 --- 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 index 23420975f284..90da4304a0b8 100644 --- 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 index 5f70eafe838a..adb7665e6bcc 100644 --- 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 index 99d872278024..ce17980c7701 100644 --- 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 index 99d87a5b7d24..47fb0c6c7729 100644 --- 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 index b3bd2e280f3c..d90d25cdfc35 100644 --- 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.