diff --git a/pkg/sql/alter_table.go b/pkg/sql/alter_table.go index 23346a334301..00bd54ef9cbf 100644 --- a/pkg/sql/alter_table.go +++ b/pkg/sql/alter_table.go @@ -19,6 +19,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/clusterversion" "github.com/cockroachdb/cockroach/pkg/keys" + "github.com/cockroachdb/cockroach/pkg/security" "github.com/cockroachdb/cockroach/pkg/server/telemetry" "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgcode" "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror" @@ -1395,10 +1396,11 @@ func injectTableStats( func (p *planner) removeColumnComment( ctx context.Context, tableID sqlbase.ID, columnID sqlbase.ColumnID, ) error { - _, err := p.ExtendedEvalContext().ExecCfg.InternalExecutor.Exec( + _, err := p.ExtendedEvalContext().ExecCfg.InternalExecutor.ExecEx( ctx, "delete-column-comment", p.txn, + sqlbase.InternalExecutorSessionDataOverride{User: security.RootUser}, "DELETE FROM system.comments WHERE type=$1 AND object_id=$2 AND sub_id=$3", keys.ColumnCommentType, tableID, diff --git a/pkg/sql/comment_on_column.go b/pkg/sql/comment_on_column.go index 592c1b568e22..51a1356c6017 100644 --- a/pkg/sql/comment_on_column.go +++ b/pkg/sql/comment_on_column.go @@ -14,8 +14,10 @@ import ( "context" "github.com/cockroachdb/cockroach/pkg/keys" + "github.com/cockroachdb/cockroach/pkg/security" "github.com/cockroachdb/cockroach/pkg/sql/privilege" "github.com/cockroachdb/cockroach/pkg/sql/sem/tree" + "github.com/cockroachdb/cockroach/pkg/sql/sqlbase" ) type commentOnColumnNode struct { @@ -49,10 +51,11 @@ func (n *commentOnColumnNode) startExec(params runParams) error { } if n.n.Comment != nil { - _, err := params.p.extendedEvalCtx.ExecCfg.InternalExecutor.Exec( + _, err := params.p.extendedEvalCtx.ExecCfg.InternalExecutor.ExecEx( params.ctx, "set-column-comment", params.p.Txn(), + sqlbase.InternalExecutorSessionDataOverride{User: security.RootUser}, "UPSERT INTO system.comments VALUES ($1, $2, $3, $4)", keys.ColumnCommentType, n.tableDesc.ID, @@ -62,10 +65,11 @@ func (n *commentOnColumnNode) startExec(params runParams) error { return err } } else { - _, err := params.p.extendedEvalCtx.ExecCfg.InternalExecutor.Exec( + _, err := params.p.extendedEvalCtx.ExecCfg.InternalExecutor.ExecEx( params.ctx, "delete-column-comment", params.p.Txn(), + sqlbase.InternalExecutorSessionDataOverride{User: security.RootUser}, "DELETE FROM system.comments WHERE type=$1 AND object_id=$2 AND sub_id=$3", keys.ColumnCommentType, n.tableDesc.ID, diff --git a/pkg/sql/comment_on_database.go b/pkg/sql/comment_on_database.go index 924abfaeef92..215231460cf0 100644 --- a/pkg/sql/comment_on_database.go +++ b/pkg/sql/comment_on_database.go @@ -14,6 +14,7 @@ import ( "context" "github.com/cockroachdb/cockroach/pkg/keys" + "github.com/cockroachdb/cockroach/pkg/security" "github.com/cockroachdb/cockroach/pkg/sql/privilege" "github.com/cockroachdb/cockroach/pkg/sql/sem/tree" "github.com/cockroachdb/cockroach/pkg/sql/sqlbase" @@ -44,10 +45,11 @@ func (p *planner) CommentOnDatabase( func (n *commentOnDatabaseNode) startExec(params runParams) error { if n.n.Comment != nil { - _, err := params.p.extendedEvalCtx.ExecCfg.InternalExecutor.Exec( + _, err := params.p.extendedEvalCtx.ExecCfg.InternalExecutor.ExecEx( params.ctx, "set-db-comment", params.p.Txn(), + sqlbase.InternalExecutorSessionDataOverride{User: security.RootUser}, "UPSERT INTO system.comments VALUES ($1, $2, 0, $3)", keys.DatabaseCommentType, n.dbDesc.ID, @@ -56,10 +58,11 @@ func (n *commentOnDatabaseNode) startExec(params runParams) error { return err } } else { - _, err := params.p.extendedEvalCtx.ExecCfg.InternalExecutor.Exec( + _, err := params.p.extendedEvalCtx.ExecCfg.InternalExecutor.ExecEx( params.ctx, "delete-db-comment", params.p.Txn(), + sqlbase.InternalExecutorSessionDataOverride{User: security.RootUser}, "DELETE FROM system.comments WHERE type=$1 AND object_id=$2 AND sub_id=0", keys.DatabaseCommentType, n.dbDesc.ID) diff --git a/pkg/sql/comment_on_index.go b/pkg/sql/comment_on_index.go index 535b34d8552d..8e861e1f9332 100644 --- a/pkg/sql/comment_on_index.go +++ b/pkg/sql/comment_on_index.go @@ -14,6 +14,7 @@ import ( "context" "github.com/cockroachdb/cockroach/pkg/keys" + "github.com/cockroachdb/cockroach/pkg/security" "github.com/cockroachdb/cockroach/pkg/sql/privilege" "github.com/cockroachdb/cockroach/pkg/sql/sem/tree" "github.com/cockroachdb/cockroach/pkg/sql/sqlbase" @@ -77,10 +78,11 @@ func (n *commentOnIndexNode) startExec(params runParams) error { func (p *planner) upsertIndexComment( ctx context.Context, tableID sqlbase.ID, indexID sqlbase.IndexID, comment string, ) error { - _, err := p.extendedEvalCtx.ExecCfg.InternalExecutor.Exec( + _, err := p.extendedEvalCtx.ExecCfg.InternalExecutor.ExecEx( ctx, "set-index-comment", p.Txn(), + sqlbase.InternalExecutorSessionDataOverride{User: security.RootUser}, "UPSERT INTO system.comments VALUES ($1, $2, $3, $4)", keys.IndexCommentType, tableID, @@ -93,10 +95,11 @@ func (p *planner) upsertIndexComment( func (p *planner) removeIndexComment( ctx context.Context, tableID sqlbase.ID, indexID sqlbase.IndexID, ) error { - _, err := p.ExtendedEvalContext().ExecCfg.InternalExecutor.Exec( + _, err := p.ExtendedEvalContext().ExecCfg.InternalExecutor.ExecEx( ctx, "delete-index-comment", p.txn, + sqlbase.InternalExecutorSessionDataOverride{User: security.RootUser}, "DELETE FROM system.comments WHERE type=$1 AND object_id=$2 AND sub_id=$3", keys.IndexCommentType, tableID, diff --git a/pkg/sql/comment_on_table.go b/pkg/sql/comment_on_table.go index 0e10d9ca4c0e..1fa705cbc858 100644 --- a/pkg/sql/comment_on_table.go +++ b/pkg/sql/comment_on_table.go @@ -14,8 +14,10 @@ import ( "context" "github.com/cockroachdb/cockroach/pkg/keys" + "github.com/cockroachdb/cockroach/pkg/security" "github.com/cockroachdb/cockroach/pkg/sql/privilege" "github.com/cockroachdb/cockroach/pkg/sql/sem/tree" + "github.com/cockroachdb/cockroach/pkg/sql/sqlbase" ) type commentOnTableNode struct { @@ -42,10 +44,11 @@ func (p *planner) CommentOnTable(ctx context.Context, n *tree.CommentOnTable) (p func (n *commentOnTableNode) startExec(params runParams) error { if n.n.Comment != nil { - _, err := params.p.extendedEvalCtx.ExecCfg.InternalExecutor.Exec( + _, err := params.p.extendedEvalCtx.ExecCfg.InternalExecutor.ExecEx( params.ctx, "set-table-comment", params.p.Txn(), + sqlbase.InternalExecutorSessionDataOverride{User: security.RootUser}, "UPSERT INTO system.comments VALUES ($1, $2, 0, $3)", keys.TableCommentType, n.tableDesc.ID, @@ -54,10 +57,11 @@ func (n *commentOnTableNode) startExec(params runParams) error { return err } } else { - _, err := params.p.extendedEvalCtx.ExecCfg.InternalExecutor.Exec( + _, err := params.p.extendedEvalCtx.ExecCfg.InternalExecutor.ExecEx( params.ctx, "delete-table-comment", params.p.Txn(), + sqlbase.InternalExecutorSessionDataOverride{User: security.RootUser}, "DELETE FROM system.comments WHERE type=$1 AND object_id=$2 AND sub_id=0", keys.TableCommentType, n.tableDesc.ID) diff --git a/pkg/sql/drop_database.go b/pkg/sql/drop_database.go index 4edb3e1d3c4c..626f98026cea 100644 --- a/pkg/sql/drop_database.go +++ b/pkg/sql/drop_database.go @@ -17,6 +17,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/internal/client" "github.com/cockroachdb/cockroach/pkg/jobs/jobspb" "github.com/cockroachdb/cockroach/pkg/keys" + "github.com/cockroachdb/cockroach/pkg/security" "github.com/cockroachdb/cockroach/pkg/server/telemetry" "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgcode" "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror" @@ -259,10 +260,11 @@ func (p *planner) accumulateDependentTables( } func (p *planner) removeDbComment(ctx context.Context, dbID sqlbase.ID) error { - _, err := p.ExtendedEvalContext().ExecCfg.InternalExecutor.Exec( + _, err := p.ExtendedEvalContext().ExecCfg.InternalExecutor.ExecEx( ctx, "delete-db-comment", p.txn, + sqlbase.InternalExecutorSessionDataOverride{User: security.RootUser}, "DELETE FROM system.comments WHERE type=$1 AND object_id=$2 AND sub_id=0", keys.DatabaseCommentType, dbID) diff --git a/pkg/sql/drop_table.go b/pkg/sql/drop_table.go index 12fb375ab8cf..b97c9168d6e5 100644 --- a/pkg/sql/drop_table.go +++ b/pkg/sql/drop_table.go @@ -18,6 +18,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/jobs/jobspb" "github.com/cockroachdb/cockroach/pkg/keys" "github.com/cockroachdb/cockroach/pkg/roachpb" + "github.com/cockroachdb/cockroach/pkg/security" "github.com/cockroachdb/cockroach/pkg/server/telemetry" "github.com/cockroachdb/cockroach/pkg/sql/privilege" "github.com/cockroachdb/cockroach/pkg/sql/sem/tree" @@ -594,10 +595,11 @@ func removeMatchingReferences( func (p *planner) removeTableComment( ctx context.Context, tableDesc *sqlbase.MutableTableDescriptor, ) error { - _, err := p.ExtendedEvalContext().ExecCfg.InternalExecutor.Exec( + _, err := p.ExtendedEvalContext().ExecCfg.InternalExecutor.ExecEx( ctx, "delete-table-comment", p.txn, + sqlbase.InternalExecutorSessionDataOverride{User: security.RootUser}, "DELETE FROM system.comments WHERE type=$1 AND object_id=$2 AND sub_id=0", keys.TableCommentType, tableDesc.ID) @@ -605,10 +607,11 @@ func (p *planner) removeTableComment( return err } - _, err = p.ExtendedEvalContext().ExecCfg.InternalExecutor.Exec( + _, err = p.ExtendedEvalContext().ExecCfg.InternalExecutor.ExecEx( ctx, "delete-comment", p.txn, + sqlbase.InternalExecutorSessionDataOverride{User: security.RootUser}, "DELETE FROM system.comments WHERE type=$1 AND object_id=$2", keys.ColumnCommentType, tableDesc.ID) diff --git a/pkg/sql/logictest/testdata/logic_test/privileges_comments b/pkg/sql/logictest/testdata/logic_test/privileges_comments new file mode 100644 index 000000000000..d4138e7b34ce --- /dev/null +++ b/pkg/sql/logictest/testdata/logic_test/privileges_comments @@ -0,0 +1,82 @@ +# Disable automatic stats to avoid flakiness. +statement ok +SET CLUSTER SETTING sql.stats.automatic_collection.enabled = false + +subtest regression45707 + +user root + +statement ok +CREATE DATABASE d45707; CREATE TABLE d45707.t45707(x INT); + GRANT SELECT ON DATABASE d45707 TO testuser; + GRANT SELECT ON d45707.t45707 TO testuser + +statement ok +COMMENT ON DATABASE d45707 IS 'd45707'; +COMMENT ON TABLE d45707.t45707 IS 't45707'; +COMMENT ON COLUMN d45707.t45707.x IS 'x45707'; +COMMENT ON INDEX d45707.t45707@primary IS 'p45707' + +user testuser + +statement ok +SET DATABASE = d45707 + +# Verify the user cannot modify the comments + +statement error user testuser does not have CREATE privilege on database d45707 +COMMENT ON DATABASE d45707 IS 'd45707' + +statement error user testuser does not have CREATE privilege on relation t45707 +COMMENT ON TABLE d45707.t45707 IS 't45707' + +statement error user testuser does not have CREATE privilege on relation t45707 +COMMENT ON COLUMN d45707.t45707.x IS 'x45707' + +statement error user testuser does not have CREATE privilege on relation t45707 +COMMENT ON INDEX d45707.t45707@primary IS 'p45707' + +# Verify that the user can view the comments + +query T +SELECT shobj_description(oid, 'pg_database') + FROM pg_database + WHERE datname = 'd45707' +---- +d45707 + +query T +SELECT col_description(attrelid, attnum) + FROM pg_attribute + WHERE attrelid = 't45707'::regclass AND attname = 'x' +---- +x45707 + +query T +SELECT obj_description('t45707'::REGCLASS) +---- +t45707 + +query T +SELECT obj_description(indexrelid) + FROM pg_index + WHERE indrelid = 't45707'::REGCLASS + AND indisprimary +---- +p45707 + +# Verify that the user can modify the comments. + +user root + +statement ok +GRANT ALL ON DATABASE d45707 TO testuser; + GRANT ALL ON TABLE d45707.t45707 TO testuser + +user testuser + +statement ok +COMMENT ON DATABASE d45707 IS 'd45707_2'; +COMMENT ON TABLE d45707.t45707 IS 't45707_2'; +COMMENT ON COLUMN d45707.t45707.x IS 'x45707_2'; +COMMENT ON INDEX d45707.t45707@primary IS 'p45707_2' diff --git a/pkg/sql/sem/builtins/pg_builtins.go b/pkg/sql/sem/builtins/pg_builtins.go index 291e3b011b2c..159e1634adea 100644 --- a/pkg/sql/sem/builtins/pg_builtins.go +++ b/pkg/sql/sem/builtins/pg_builtins.go @@ -837,9 +837,17 @@ var pgBuiltins = map[string]builtinDefinition{ // below to pick up the table comment by accident. return tree.DNull, nil } + // Note: the following is equivalent to: + // + // SELECT description FROM pg_catalog.pg_description + // WHERE objoid=$1 AND objsubid=$2 LIMIT 1 + // + // TODO(jordanlewis): Really we'd like to query this directly + // on pg_description and let predicate push-down do its job. r, err := ctx.InternalExecutor.QueryRow( ctx.Ctx(), "pg_get_coldesc", - ctx.Txn, ` + ctx.Txn, + ` SELECT COALESCE(c.comment, pc.comment) FROM system.comments c FULL OUTER JOIN crdb_internal.predefined_comments pc ON pc.object_id=c.object_id AND pc.sub_id=c.sub_id AND pc.type = c.type diff --git a/pkg/sql/sqlbase/system.go b/pkg/sql/sqlbase/system.go index 30b56bf765bc..f7f0af03a486 100644 --- a/pkg/sql/sqlbase/system.go +++ b/pkg/sql/sqlbase/system.go @@ -1554,6 +1554,7 @@ func IsReservedID(id ID) bool { // newCommentPrivilegeDescriptor returns a privilege descriptor for comment table func newCommentPrivilegeDescriptor(priv privilege.List) *PrivilegeDescriptor { + selectPriv := privilege.List{privilege.SELECT} return &PrivilegeDescriptor{ Users: []UserPrivileges{ { @@ -1562,7 +1563,7 @@ func newCommentPrivilegeDescriptor(priv privilege.List) *PrivilegeDescriptor { }, { User: PublicRole, - Privileges: priv.ToBitField(), + Privileges: selectPriv.ToBitField(), }, { User: security.RootUser, diff --git a/pkg/sql/truncate.go b/pkg/sql/truncate.go index 8495d01c8b81..5d979699d2cc 100644 --- a/pkg/sql/truncate.go +++ b/pkg/sql/truncate.go @@ -466,10 +466,11 @@ func reassignColumnComment( } if comment != nil { - _, err = p.ExtendedEvalContext().ExecCfg.InternalExecutor.Exec( + _, err = p.ExtendedEvalContext().ExecCfg.InternalExecutor.ExecEx( ctx, "set-column-comment", p.txn, + sqlbase.InternalExecutorSessionDataOverride{User: security.RootUser}, "UPSERT INTO system.comments VALUES ($1, $2, $3, $4)", keys.ColumnCommentType, newID, @@ -479,10 +480,11 @@ func reassignColumnComment( return err } - _, err = p.ExtendedEvalContext().ExecCfg.InternalExecutor.Exec( + _, err = p.ExtendedEvalContext().ExecCfg.InternalExecutor.ExecEx( ctx, "delete-column-comment", p.txn, + sqlbase.InternalExecutorSessionDataOverride{User: security.RootUser}, "DELETE FROM system.comments WHERE type=$1 AND object_id=$2 AND sub_id=$3", keys.ColumnCommentType, oldID, diff --git a/pkg/sqlmigrations/migrations.go b/pkg/sqlmigrations/migrations.go index f89b0b5e04a4..c8073cbb016c 100644 --- a/pkg/sqlmigrations/migrations.go +++ b/pkg/sqlmigrations/migrations.go @@ -291,7 +291,7 @@ var backwardCompatibleMigrations = []migrationDescriptor{ includedInBootstrap: clusterversion.VersionByKey(clusterversion.VersionNamespaceTableWithSchemas), }, { - // Introduced in v20.1., + // Introduced in v20.1. name: "create system.role_options table with an entry for (admin, CREATEROLE)", workFn: createRoleOptionsTable, includedInBootstrap: clusterversion.VersionByKey(clusterversion.VersionCreateRolePrivilege), @@ -307,6 +307,11 @@ var backwardCompatibleMigrations = []migrationDescriptor{ newDescriptorIDs: staticIDs(keys.StatementBundleChunksTableID, keys.StatementDiagnosticsRequestsTableID, keys.StatementDiagnosticsTableID), }, + { + // Introduced in v20.1. + name: "remove public permissions on system.comments", + workFn: depublicizeSystemComments, + }, } func staticIDs(ids ...sqlbase.ID) func(ctx context.Context, db db) ([]sqlbase.ID, error) { @@ -1093,3 +1098,19 @@ func updateSystemLocationData(ctx context.Context, r runner) error { } return nil } + +func depublicizeSystemComments(ctx context.Context, r runner) error { + // At some point in time, system.comments was mistakenly created + // with all privileges granted to the "public" role (i.e. everyone). + // This migration cleans this up. + + for _, priv := range []string{"GRANT", "INSERT", "DELETE", "UPDATE"} { + stmt := fmt.Sprintf(`REVOKE %s ON TABLE system.comments FROM public`, priv) + // REVOKE should never fail here -- it's always possible for root + // to revoke a privilege even if it's not currently granted. + if err := r.execAsRoot(ctx, "depublicize-system-comments", stmt); err != nil { + return err + } + } + return nil +}