From 732fcb5a3511fba84a266052de3c699e9c79677c Mon Sep 17 00:00:00 2001 From: Solon Gordon Date: Tue, 21 Jul 2020 12:07:44 -0400 Subject: [PATCH 1/2] rfcs: new RFC on finer-grained role options Release note: None --- .../20200720_finer_grained_role_privileges.md | 156 ++++++++++++++++++ 1 file changed, 156 insertions(+) create mode 100644 docs/RFCS/20200720_finer_grained_role_privileges.md diff --git a/docs/RFCS/20200720_finer_grained_role_privileges.md b/docs/RFCS/20200720_finer_grained_role_privileges.md new file mode 100644 index 000000000000..18d4f15dcca8 --- /dev/null +++ b/docs/RFCS/20200720_finer_grained_role_privileges.md @@ -0,0 +1,156 @@ +- Feature Name: Finer-grained Role Privileges +- Status: draft +- Start Date: 2020-07-21 +- Authors: Solon Gordon, Joel Kenny + +# Summary + +CockroachDB offers a built-in "admin" role which can be granted in order to +give users a wide range of privileges. For one thing, it grants full +permissions on all objects in the database. But there are also a number of +operations which only admins are permitted to perform, like creating a new +database or changing cluster settings. Since these operations can only be +performed by admins, it's currently impossible to grant a user the ability to +perform any one of these operations without giving them full admin access. This +document proposes adding new role-level privileges so that admin-like abilities +can be granted in a more fine-grained manner. + +# Motivation + +One major motivator for these changes is CockroachCloud. When we create the +initial user for a CockroachCloud cluster, we currently grant the admin role +to that user so they can perform basic operations like creating new databases. +However, this also gives that user the ability to perform actions which could +damage their cluster, like deleting the user which is used for automatic +backup. Rather than granting the admin role, the CockroachCloud team would like +the ability to create an "operator" role which only grants whatever abilities +are necessary for the user to administer their cluster. + +These new options will also be useful for customers who want to grant users +administrative abilities without managing the security of their cluster by +limiting the number of people with true admin access. + +# Guide-level explanation + +As of v20.1, CockroachDB supports a few role-level options which can be granted +via the `CREATE ROLE` or `ALTER ROLE` statement. One example is `CREATEROLE`, +which grants users the ability to create, alter, and drop other roles. A +complete list is available at +https://www.cockroachlabs.com/docs/v20.1/alter-role.html#parameters. + +In order to support granting admin-like abilities, we add several new role +options: + +### CREATEDB +Allows a user to create new databases. The user who issues the command is the +owner of the database in question. This confers the ability to rename the +database via `ALTER DATABASE ... RENAME`. + +### CREATELOGIN +Allows a user to manage authentication. This grants access to: +* the `WITH PASSWORD` clause for `CREATE/ALTER USER/ROLE` +* the `VALID UNTIL` clause for `CREATE/ALTER USER/ROLE` +* `ALTER USER/ROLE CREATELOGIN/NOCREATELOGIN` +* `ALTER USER/ROLE LOGIN/NOLOGIN` + +Note that these abilities were previously available to users with the +CREATEROLE option. For backward compatibility, a 20.2 migration grants +CREATELOGIN to all users who previously had CREATEROLE. + +### CREATEROLE +As noted above, we already support this option. However, it currently does not +prevent a non-admin user from dropping an admin. We will add this restriction +to protect against dropping essential admin users. + +### CONTROLJOB +Allows a user to pause, resume, and cancel jobs. Non-admin users cannot control +jobs created by admins. + +### CANCELQUERY +Allows users to cancel queries and sessions of other users. Without this +privilege, users can only cancel their own queries and sessions. Even with this +privilege, non-admins cannot cancel admin queries or sessions. This option +should usually be combined with VIEWACTIVITY so that the user can view other +user's query and session information. + +### VIEWACTIVITY +Allows a user to see other users' queries and sessions via the following means: +- SHOW QUERIES +- SHOW SESSIONS +- the WebUI Statements and Transactions pages +Without this privilege, the SHOW commands only show the user's own data, and the +WebUI pages are unavailable. + +### CONTROLCHANGEFEED +Allows users to run `CREATE CHANGEFEED` on tables they have `SELECT` privileges +on. In the future this should also control whether a user can pause, resume, or +cancel a changefeed, but that is currently controlled via job control so will +be determined by the `CONTROLJOB` option. + +### MODIFYCLUSTERSETTING +Allows a user to modify certain cluster settings. At present this allows +modifying any setting with the `sql.defaults` prefix, but this list may be +expanded in the future. + +## BACKUP, RESTORE, and IMPORT INTO +We also add the ability for non-admin users to perform certain BACKUP, RESTORE, +and IMPORT INTO operations. Rather than enabling this via a new role option, we +do so based on existing privileges. Database and table backups require SELECT +privileges on the target object. User-defined type and schema backups require +the USAGE privilege. + +RESTOREs require CREATE privileges on all restored objects and CREATEDB in the +case of database restores. + +IMPORT INTO requires INSERT and DROP on the target table. (DROP is required +because the IMPORT implementation makes the table unavailable for the duration +of the operation.) + +Full cluster and tenant BACKUP and RESTORE continue to require admin. + +We also restrict what source URLs non-admins can use for these operations. +nodelocal, HTTP, and AWS/GCS/Azure sources which rely on implicit credentials +will continue to require the admin role. This is acceptable for the Cockroach +Cloud use case since that system uses explicit, temporary credentials for +RESTORE. + +# Reference-level explanation +The backend implementation of these new privileges should be straightforward. +Role-level options are already stored in the `system.role_options` table and no +migration is necessary to add new options. We will add the new privileges to +our list of supported role options and assert that a user has the appropriate +role option (or is an admin) when performing the associated operation. + +# Limitations + +## Automatic grants +The new privileges proposed here do not include a way to guarantee that a role +has privileges on all objects in the cluster. This remains a unique property of +the admin role. + +This may present a problem for CockroachCloud, since the customer will not have +access to any admin users, so there will be no user who has reliable access to +all objects. However, adding a mechanism for granting non-admins cluster-wide +access is also undesirable, because there are some databases and tables which +we don't want them to have access to. One example of this is tables which we +use to manage automatic backups. + +However, the CockroachCloud team could mitigate this by enhancing their +management console to allow operators to modify privileges. For example, the +console could allow altering the owner of database objects. That way a user +could switch the owner to themself or the operator role if they needed access +to a given object. Behind the scenes, these privileges changes would be +performed by an admin user which is not exposed to the customer. + +# Alternatives + +### CONTROLCHANGEFEED +Rather than making CONTROLCHANGEFEED a role option, we could consider making it +a privilege at the database/schema/table level. This would provide more granular +control but seems less desirable in a few ways: +* Since the owner of an object receives all privileges on it, creating a table + would give a user the ability to create a changefeed, which might not be + desired. +* It's not clear if there is a realistic scenario where a user should only have + the ability to create changefeeds on only certain tables they have read + access to. From 328ef659e9a99fb3873e22d0542385a23d04fe8c Mon Sep 17 00:00:00 2001 From: richardjcai Date: Wed, 22 Sep 2021 15:42:51 -0400 Subject: [PATCH 2/2] sql: fix error message for currval, nextval and setval for sequences Release note (sql change): Small fix to error messaging for builtin functions for sequences. Example: SELECT nextval('@#%@!324234') correctly returns relation "@#%@!324234" does not exist (if the relation doesn't exist) instead of a syntax error. SELECT currval('') returns currval\(\): invalid table name: instead of pq: currval(): syntax error at or near "to" DETAIL: source SQL: ALTER TABLE RENAME TO x ^ HINT: try \h ALTER TABLE --- pkg/ccl/importccl/import_table_creation.go | 21 ---------- pkg/sql/faketreeeval/evalctx.go | 22 ---------- .../logictest/testdata/logic_test/sequences | 12 ++++-- pkg/sql/sem/builtins/builtins.go | 21 +++++----- pkg/sql/sem/tree/eval.go | 15 ------- pkg/sql/sequence.go | 42 ------------------- 6 files changed, 18 insertions(+), 115 deletions(-) diff --git a/pkg/ccl/importccl/import_table_creation.go b/pkg/ccl/importccl/import_table_creation.go index e68fd259a228..4f1e5ca1dd0c 100644 --- a/pkg/ccl/importccl/import_table_creation.go +++ b/pkg/ccl/importccl/import_table_creation.go @@ -379,13 +379,6 @@ func (so *importSequenceOperators) HasPrivilege( return false, errors.WithStack(errSequenceOperators) } -// IncrementSequence implements the tree.SequenceOperators interface. -func (so *importSequenceOperators) IncrementSequence( - ctx context.Context, seqName *tree.TableName, -) (int64, error) { - return 0, errSequenceOperators -} - // IncrementSequenceByID implements the tree.SequenceOperators interface. func (so *importSequenceOperators) IncrementSequenceByID( ctx context.Context, seqID int64, @@ -393,13 +386,6 @@ func (so *importSequenceOperators) IncrementSequenceByID( return 0, errSequenceOperators } -// GetLatestValueInSessionForSequence implements the tree.SequenceOperators interface. -func (so *importSequenceOperators) GetLatestValueInSessionForSequence( - ctx context.Context, seqName *tree.TableName, -) (int64, error) { - return 0, errSequenceOperators -} - // GetLatestValueInSessionForSequenceByID implements the tree.SequenceOperators interface. func (so *importSequenceOperators) GetLatestValueInSessionForSequenceByID( ctx context.Context, seqID int64, @@ -407,13 +393,6 @@ func (so *importSequenceOperators) GetLatestValueInSessionForSequenceByID( return 0, errSequenceOperators } -// SetSequenceValue implements the tree.SequenceOperators interface. -func (so *importSequenceOperators) SetSequenceValue( - ctx context.Context, seqName *tree.TableName, newVal int64, isCalled bool, -) error { - return errSequenceOperators -} - // SetSequenceValueByID implements the tree.SequenceOperators interface. func (so *importSequenceOperators) SetSequenceValueByID( ctx context.Context, seqID int64, newVal int64, isCalled bool, diff --git a/pkg/sql/faketreeeval/evalctx.go b/pkg/sql/faketreeeval/evalctx.go index 258c2e01c231..eac2943a6295 100644 --- a/pkg/sql/faketreeeval/evalctx.go +++ b/pkg/sql/faketreeeval/evalctx.go @@ -89,13 +89,6 @@ func (so *DummySequenceOperators) HasPrivilege( return false, errors.WithStack(errEvalPlanner) } -// IncrementSequence is part of the tree.SequenceOperators interface. -func (so *DummySequenceOperators) IncrementSequence( - ctx context.Context, seqName *tree.TableName, -) (int64, error) { - return 0, errors.WithStack(errSequenceOperators) -} - // IncrementSequenceByID is part of the tree.SequenceOperators interface. func (so *DummySequenceOperators) IncrementSequenceByID( ctx context.Context, seqID int64, @@ -103,14 +96,6 @@ func (so *DummySequenceOperators) IncrementSequenceByID( return 0, errors.WithStack(errSequenceOperators) } -// GetLatestValueInSessionForSequence implements the tree.SequenceOperators -// interface. -func (so *DummySequenceOperators) GetLatestValueInSessionForSequence( - ctx context.Context, seqName *tree.TableName, -) (int64, error) { - return 0, errors.WithStack(errSequenceOperators) -} - // GetLatestValueInSessionForSequenceByID implements the tree.SequenceOperators // interface. func (so *DummySequenceOperators) GetLatestValueInSessionForSequenceByID( @@ -119,13 +104,6 @@ func (so *DummySequenceOperators) GetLatestValueInSessionForSequenceByID( return 0, errors.WithStack(errSequenceOperators) } -// SetSequenceValue implements the tree.SequenceOperators interface. -func (so *DummySequenceOperators) SetSequenceValue( - ctx context.Context, seqName *tree.TableName, newVal int64, isCalled bool, -) error { - return errors.WithStack(errSequenceOperators) -} - // SetSequenceValueByID implements the tree.SequenceOperators interface. func (so *DummySequenceOperators) SetSequenceValueByID( ctx context.Context, seqID int64, newVal int64, isCalled bool, diff --git a/pkg/sql/logictest/testdata/logic_test/sequences b/pkg/sql/logictest/testdata/logic_test/sequences index f1f6450a6a96..a72a9eb60d71 100644 --- a/pkg/sql/logictest/testdata/logic_test/sequences +++ b/pkg/sql/logictest/testdata/logic_test/sequences @@ -222,7 +222,7 @@ SELECT foo from select_test # USING THE `nextval` AND `currval` FUNCTIONS -statement error pgcode 55000 pq: currval\(\): currval of sequence "foo" is not yet defined in this session +statement error pgcode 55000 pq: currval\(\): currval of sequence "test.public.foo" is not yet defined in this session SELECT currval('foo') query I @@ -350,11 +350,11 @@ SELECT nextval('spécial') statement ok CREATE TABLE kv (k bytes primary key, v bytes) -statement error pgcode 42809 "kv" is not a sequence +statement error pgcode 42809 nextval\(\): "test.public.kv" is not a sequence SELECT nextval('kv') -# Parse errors in the argument to nextval are handled. -statement error pq: nextval\(\): at or near "@": syntax error +# This matches PG's error. +statement error pq: nextval\(\): relation "@#%@!324234" does not exist SELECT nextval('@#%@!324234') # You can create and find sequences from other databases. @@ -1816,3 +1816,7 @@ SELECT * FROM "".crdb_internal.cross_db_references; db2 public seq db1 public t sequences owning table db2 public seq2 db1 public t sequences owning table test public tdb3ref db3 public s table column refers to sequence + +# Testing parsing empty string for currval issue #34527. +statement error pq: currval\(\): invalid table name: +SELECT currval('') diff --git a/pkg/sql/sem/builtins/builtins.go b/pkg/sql/sem/builtins/builtins.go index 176f1688b77d..425051908047 100644 --- a/pkg/sql/sem/builtins/builtins.go +++ b/pkg/sql/sem/builtins/builtins.go @@ -2026,11 +2026,11 @@ var builtins = map[string]builtinDefinition{ ReturnType: tree.FixedReturnType(types.Int), Fn: func(evalCtx *tree.EvalContext, args tree.Datums) (tree.Datum, error) { name := tree.MustBeDString(args[0]) - qualifiedName, err := parser.ParseQualifiedTableName(string(name)) + dOid, err := tree.ParseDOid(evalCtx, string(name), types.RegClass) if err != nil { return nil, err } - res, err := evalCtx.Sequence.IncrementSequence(evalCtx.Ctx(), qualifiedName) + res, err := evalCtx.Sequence.IncrementSequenceByID(evalCtx.Ctx(), int64(dOid.DInt)) if err != nil { return nil, err } @@ -2066,11 +2066,11 @@ var builtins = map[string]builtinDefinition{ ReturnType: tree.FixedReturnType(types.Int), Fn: func(evalCtx *tree.EvalContext, args tree.Datums) (tree.Datum, error) { name := tree.MustBeDString(args[0]) - qualifiedName, err := parser.ParseQualifiedTableName(string(name)) + dOid, err := tree.ParseDOid(evalCtx, string(name), types.RegClass) if err != nil { return nil, err } - res, err := evalCtx.Sequence.GetLatestValueInSessionForSequence(evalCtx.Ctx(), qualifiedName) + res, err := evalCtx.Sequence.GetLatestValueInSessionForSequenceByID(evalCtx.Ctx(), int64(dOid.DInt)) if err != nil { return nil, err } @@ -2127,14 +2127,14 @@ var builtins = map[string]builtinDefinition{ ReturnType: tree.FixedReturnType(types.Int), Fn: func(evalCtx *tree.EvalContext, args tree.Datums) (tree.Datum, error) { name := tree.MustBeDString(args[0]) - qualifiedName, err := parser.ParseQualifiedTableName(string(name)) + dOid, err := tree.ParseDOid(evalCtx, string(name), types.RegClass) if err != nil { return nil, err } newVal := tree.MustBeDInt(args[1]) - if err := evalCtx.Sequence.SetSequenceValue( - evalCtx.Ctx(), qualifiedName, int64(newVal), true); err != nil { + if err := evalCtx.Sequence.SetSequenceValueByID( + evalCtx.Ctx(), int64(dOid.DInt), int64(newVal), true); err != nil { return nil, err } return args[1], nil @@ -2166,16 +2166,15 @@ var builtins = map[string]builtinDefinition{ ReturnType: tree.FixedReturnType(types.Int), Fn: func(evalCtx *tree.EvalContext, args tree.Datums) (tree.Datum, error) { name := tree.MustBeDString(args[0]) - qualifiedName, err := parser.ParseQualifiedTableName(string(name)) + dOid, err := tree.ParseDOid(evalCtx, string(name), types.RegClass) if err != nil { return nil, err } - isCalled := bool(tree.MustBeDBool(args[2])) newVal := tree.MustBeDInt(args[1]) - if err := evalCtx.Sequence.SetSequenceValue( - evalCtx.Ctx(), qualifiedName, int64(newVal), isCalled); err != nil { + if err := evalCtx.Sequence.SetSequenceValueByID( + evalCtx.Ctx(), int64(dOid.DInt), int64(newVal), isCalled); err != nil { return nil, err } return args[1], nil diff --git a/pkg/sql/sem/tree/eval.go b/pkg/sql/sem/tree/eval.go index 26a3989b5914..f337f2553d2d 100644 --- a/pkg/sql/sem/tree/eval.go +++ b/pkg/sql/sem/tree/eval.go @@ -3340,21 +3340,6 @@ type SequenceOperators interface { // Returns an empty string if the sequence name does not exist. GetSerialSequenceNameFromColumn(ctx context.Context, tableName *TableName, columnName Name) (*TableName, error) - // IncrementSequence increments the given sequence and returns the result. - // It returns an error if the given name is not a sequence. - // The caller must ensure that seqName is fully qualified already. - IncrementSequence(ctx context.Context, seqName *TableName) (int64, error) - - // GetLatestValueInSessionForSequence returns the value most recently obtained by - // nextval() for the given sequence in this session. - GetLatestValueInSessionForSequence(ctx context.Context, seqName *TableName) (int64, error) - - // SetSequenceValue sets the sequence's value. - // If isCalled is false, the sequence is set such that the next time nextval() is called, - // `newVal` is returned. Otherwise, the next call to nextval will return - // `newVal + seqOpts.Increment`. - SetSequenceValue(ctx context.Context, seqName *TableName, newVal int64, isCalled bool) error - // IncrementSequenceByID increments the given sequence and returns the result. // It returns an error if the given ID is not a sequence. // Takes in a sequence ID rather than a name, unlike IncrementSequence. diff --git a/pkg/sql/sequence.go b/pkg/sql/sequence.go index f1e6338bdee1..9714960c98c7 100644 --- a/pkg/sql/sequence.go +++ b/pkg/sql/sequence.go @@ -73,20 +73,6 @@ func (p *planner) GetSerialSequenceNameFromColumn( return nil, colinfo.NewUndefinedColumnError(string(columnName)) } -// IncrementSequence implements the tree.SequenceOperators interface. -func (p *planner) IncrementSequence(ctx context.Context, seqName *tree.TableName) (int64, error) { - if p.EvalContext().TxnReadOnly { - return 0, readOnlyError("nextval()") - } - - flags := tree.ObjectLookupFlagsWithRequiredTableKind(tree.ResolveRequireSequenceDesc) - _, descriptor, err := resolver.ResolveExistingTableObject(ctx, p, seqName, flags) - if err != nil { - return 0, err - } - return incrementSequenceHelper(ctx, p, descriptor) -} - // IncrementSequenceByID implements the tree.SequenceOperators interface. func (p *planner) IncrementSequenceByID(ctx context.Context, seqID int64) (int64, error) { if p.EvalContext().TxnReadOnly { @@ -226,18 +212,6 @@ func boundsExceededError(descriptor catalog.TableDescriptor) error { tree.ErrString((*tree.Name)(&name)), value) } -// GetLatestValueInSessionForSequence implements the tree.SequenceOperators interface. -func (p *planner) GetLatestValueInSessionForSequence( - ctx context.Context, seqName *tree.TableName, -) (int64, error) { - flags := tree.ObjectLookupFlagsWithRequiredTableKind(tree.ResolveRequireSequenceDesc) - _, descriptor, err := resolver.ResolveExistingTableObject(ctx, p, seqName, flags) - if err != nil { - return 0, err - } - return getLatestValueInSessionForSequenceHelper(p, descriptor, seqName) -} - // GetLatestValueInSessionForSequenceByID implements the tree.SequenceOperators interface. func (p *planner) GetLatestValueInSessionForSequenceByID( ctx context.Context, seqID int64, @@ -273,22 +247,6 @@ func getLatestValueInSessionForSequenceHelper( return val, nil } -// SetSequenceValue implements the tree.SequenceOperators interface. -func (p *planner) SetSequenceValue( - ctx context.Context, seqName *tree.TableName, newVal int64, isCalled bool, -) error { - if p.EvalContext().TxnReadOnly { - return readOnlyError("setval()") - } - - flags := tree.ObjectLookupFlagsWithRequiredTableKind(tree.ResolveRequireSequenceDesc) - _, descriptor, err := resolver.ResolveExistingTableObject(ctx, p, seqName, flags) - if err != nil { - return err - } - return setSequenceValueHelper(ctx, p, descriptor, newVal, isCalled, seqName) -} - // SetSequenceValueByID implements the tree.SequenceOperators interface. func (p *planner) SetSequenceValueByID( ctx context.Context, seqID int64, newVal int64, isCalled bool,