Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

sql, server: regulate access to remaining observability features #85769

Merged
merged 1 commit into from Aug 11, 2022
Merged

sql, server: regulate access to remaining observability features #85769

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
22 changes: 20 additions & 2 deletions pkg/server/admin.go
View file
Open in desktop
Expand Up @@ -248,6 +248,10 @@ func (s *adminServer) Databases(
return nil, serverError(ctx, err)
}

if err := s.requireViewActivityPermission(ctx); err != nil {
return nil, err
}

r, err := s.databasesHelper(ctx, req, sessionUser, 0, 0)
return r, maybeHandleNotFoundError(ctx, err)
}
Expand Down Expand Up @@ -314,6 +318,10 @@ func (s *adminServer) DatabaseDetails(
return nil, serverError(ctx, err)
}

if err := s.requireViewActivityPermission(ctx); err != nil {
return nil, err
}

r, err := s.databaseDetailsHelper(ctx, req, userName)
return r, maybeHandleNotFoundError(ctx, err)
}
Expand Down Expand Up @@ -678,6 +686,10 @@ func (s *adminServer) TableDetails(
return nil, serverError(ctx, err)
}

if err := s.requireViewActivityPermission(ctx); err != nil {
return nil, err
}

r, err := s.tableDetailsHelper(ctx, req, userName)
return r, maybeHandleNotFoundError(ctx, err)
}
Expand Down Expand Up @@ -1073,7 +1085,13 @@ func (s *adminServer) TableStats(
ctx context.Context, req *serverpb.TableStatsRequest,
) (*serverpb.TableStatsResponse, error) {
ctx = s.server.AnnotateCtx(ctx)
userName, err := s.requireAdminUser(ctx)

userName, err := userFromContext(ctx)
if err != nil {
return nil, serverError(ctx, err)
}

err = s.requireViewActivityPermission(ctx)
if err != nil {
// NB: not using serverError() here since the priv checker
// already returns a proper gRPC error status.
Expand Down Expand Up @@ -1103,7 +1121,7 @@ func (s *adminServer) NonTableStats(
ctx context.Context, req *serverpb.NonTableStatsRequest,
) (*serverpb.NonTableStatsResponse, error) {
ctx = s.server.AnnotateCtx(ctx)
if _, err := s.requireAdminUser(ctx); err != nil {
if err := s.requireViewActivityPermission(ctx); err != nil {
// NB: not using serverError() here since the priv checker
// already returns a proper gRPC error status.
return nil, err
Expand Down
9 changes: 9 additions & 0 deletions pkg/server/admin_test.go
View file
Open in desktop
Expand Up @@ -386,6 +386,15 @@ func TestAdminAPIDatabases(t *testing.T) {
if _, err := db.Exec(query); err != nil {
t.Fatal(err)
}
// Non admins now also require VIEWACTIVITY.
query = fmt.Sprintf(
"GRANT SYSTEM %s TO %s",
"VIEWACTIVITY",
authenticatedUserNameNoAdmin().SQLIdentifier(),
)
if _, err := db.Exec(query); err != nil {
t.Fatal(err)
}

for _, tc := range []struct {
expectedDBs []string
Expand Down
2 changes: 1 addition & 1 deletion pkg/server/index_usage_stats.go
View file
Open in desktop
Expand Up @@ -194,7 +194,7 @@ func (s *statusServer) TableIndexStats(
ctx = propagateGatewayMetadata(ctx)
ctx = s.AnnotateCtx(ctx)

if err := s.privilegeChecker.requireViewActivityOrViewActivityRedactedPermission(ctx); err != nil {
if err := s.privilegeChecker.requireViewActivityPermission(ctx); err != nil {
return nil, err
}
return getTableIndexUsageStats(ctx, req, s.sqlServer.pgServer.SQLServer.GetLocalIndexStatistics(),
Expand Down
25 changes: 23 additions & 2 deletions pkg/sql/alter_table.go
View file
Open in desktop
Expand Up @@ -18,6 +18,7 @@ import (
"sort"
"time"

"github.com/cockroachdb/cockroach/pkg/clusterversion"
"github.com/cockroachdb/cockroach/pkg/jobs"
"github.com/cockroachdb/cockroach/pkg/keys"
"github.com/cockroachdb/cockroach/pkg/security/username"
Expand All @@ -34,6 +35,7 @@ import (
"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror"
"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgnotice"
"github.com/cockroachdb/cockroach/pkg/sql/privilege"
"github.com/cockroachdb/cockroach/pkg/sql/roleoption"
"github.com/cockroachdb/cockroach/pkg/sql/sem/eval"
"github.com/cockroachdb/cockroach/pkg/sql/sem/tree"
"github.com/cockroachdb/cockroach/pkg/sql/sem/volatility"
Expand All @@ -43,6 +45,7 @@ import (
"github.com/cockroachdb/cockroach/pkg/sql/stats"
"github.com/cockroachdb/cockroach/pkg/sql/storageparam"
"github.com/cockroachdb/cockroach/pkg/sql/storageparam/tablestorageparam"
"github.com/cockroachdb/cockroach/pkg/sql/syntheticprivilege"
"github.com/cockroachdb/cockroach/pkg/sql/types"
"github.com/cockroachdb/cockroach/pkg/util/errorutil/unimplemented"
"github.com/cockroachdb/cockroach/pkg/util/log/eventpb"
Expand Down Expand Up @@ -907,10 +910,28 @@ func (p *planner) setAuditMode(
p.curPlan.auditEvents = append(p.curPlan.auditEvents,
auditEvent{desc: desc, writing: true})

// We require root for now. Later maybe use a different permission?
if err := p.RequireAdminRole(ctx, "change auditing settings on a table"); err != nil {
// Requires admin or MODIFYCLUSTERSETTING as of 22.2
hasAdmin, err := p.HasAdminRole(ctx)
if err != nil {
return false, err
}
if !hasAdmin {
// Check for system privilege first, otherwise fall back to role options.
hasModify := false
if p.ExecCfg().Settings.Version.IsActive(ctx, clusterversion.SystemPrivilegesTable) {
hasModify = p.CheckPrivilege(ctx, syntheticprivilege.GlobalPrivilegeObject, privilege.MODIFYCLUSTERSETTING) == nil
}
if !hasModify {
hasModify, err = p.HasRoleOption(ctx, roleoption.MODIFYCLUSTERSETTING)
if err != nil {
return false, err
}
if !hasModify {
return false, pgerror.Newf(pgcode.InsufficientPrivilege,
"only users with admin or %s system privilege are allowed to change audit settings on a table ", privilege.MODIFYCLUSTERSETTING.String())
}
}
}

telemetry.Inc(sqltelemetry.SchemaSetAuditModeCounter(auditMode.TelemetryName()))

Expand Down
2 changes: 1 addition & 1 deletion pkg/sql/logictest/testdata/logic_test/alter_table
View file
Open in desktop
Expand Up @@ -850,7 +850,7 @@ statement ok
ALTER TABLE audit ADD COLUMN y INT

# But not the audit settings.
statement error change auditing settings on a table
statement error pq: only users with admin or MODIFYCLUSTERSETTING system privilege are allowed to change audit settings on a table
ALTER TABLE audit EXPERIMENTAL_AUDIT SET OFF

user root
Expand Down
2 changes: 2 additions & 0 deletions pkg/sql/sessioninit/BUILD.bazel
View file
Open in desktop
Expand Up @@ -42,9 +42,11 @@ go_test(
"//pkg/security/securitytest",
"//pkg/security/username",
"//pkg/server",
"//pkg/settings/cluster",
"//pkg/sql",
"//pkg/sql/catalog/descpb",
"//pkg/sql/catalog/descs",
"//pkg/sql/sessiondatapb",
"//pkg/sql/sqlutil",
"//pkg/testutils/serverutils",
"//pkg/testutils/sqlutils",
Expand Down
7 changes: 5 additions & 2 deletions pkg/sql/sessioninit/cache.go
View file
Open in desktop
Expand Up @@ -114,10 +114,13 @@ func (a *Cache) GetAuthInfo(
ctx context.Context,
ie sqlutil.InternalExecutor,
username username.SQLUsername,
makePlanner func(opName string) (interface{}, func()),
settings *cluster.Settings,
) (AuthInfo, error),
makePlanner func(opName string) (interface{}, func()),
) (aInfo AuthInfo, err error) {
if !CacheEnabled.Get(&settings.SV) {
return readFromSystemTables(ctx, ie, username)
return readFromSystemTables(ctx, ie, username, makePlanner, settings)
}

var usersTableDesc catalog.TableDescriptor
Expand Down Expand Up @@ -164,7 +167,7 @@ func (a *Cache) GetAuthInfo(
val, err := a.loadValueOutsideOfCache(
ctx, fmt.Sprintf("authinfo-%s-%d-%d", username.Normalized(), usersTableVersion, roleOptionsTableVersion),
func(loadCtx context.Context) (interface{}, error) {
return readFromSystemTables(loadCtx, ie, username)
return readFromSystemTables(loadCtx, ie, username, makePlanner, settings)
})
if err != nil {
return aInfo, err
Expand Down
47 changes: 41 additions & 6 deletions pkg/sql/sessioninit/cache_test.go
View file
Open in desktop
Expand Up @@ -19,9 +19,11 @@ import (

"github.com/cockroachdb/cockroach/pkg/base"
"github.com/cockroachdb/cockroach/pkg/security/username"
"github.com/cockroachdb/cockroach/pkg/settings/cluster"
"github.com/cockroachdb/cockroach/pkg/sql"
"github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb"
"github.com/cockroachdb/cockroach/pkg/sql/catalog/descs"
"github.com/cockroachdb/cockroach/pkg/sql/sessiondatapb"
"github.com/cockroachdb/cockroach/pkg/sql/sessioninit"
"github.com/cockroachdb/cockroach/pkg/sql/sqlutil"
"github.com/cockroachdb/cockroach/pkg/testutils/serverutils"
Expand Down Expand Up @@ -74,18 +76,30 @@ func TestCacheInvalidation(t *testing.T) {
return settings, didReadFromSystemTable, err
}
getAuthInfoFromCache := func() (sessioninit.AuthInfo, bool, error) {
makePlanner := func(opName string) (interface{}, func()) {
return sql.NewInternalPlanner(
opName,
execCfg.DB.NewTxn(ctx, opName),
username.RootUserName(),
&sql.MemoryMetrics{},
s.ExecutorConfig().(*sql.ExecutorConfig),
sessiondatapb.SessionData{},
)
}
didReadFromSystemTable := false
settings := s.ClusterSettings()
aInfo, err := execCfg.SessionInitCache.GetAuthInfo(
ctx,
s.ClusterSettings(),
settings,
s.InternalExecutor().(sqlutil.InternalExecutor),
s.DB(),
s.CollectionFactory().(*descs.CollectionFactory),
username.TestUserName(),
func(ctx context.Context, ie sqlutil.InternalExecutor, userName username.SQLUsername) (sessioninit.AuthInfo, error) {
func(ctx context.Context, ie sqlutil.InternalExecutor, userName username.SQLUsername, makePlanner func(opName string) (interface{}, func()), settings *cluster.Settings) (sessioninit.AuthInfo, error) {
didReadFromSystemTable = true
return sessioninit.AuthInfo{}, nil
})
},
makePlanner)
return aInfo, didReadFromSystemTable, err
}

Expand Down Expand Up @@ -202,6 +216,7 @@ func TestCacheSingleFlight(t *testing.T) {
ctx := context.Background()
s, db, _ := serverutils.StartServer(t, base.TestServerArgs{})
defer s.Stopper().Stop(ctx)
execCfg := s.ExecutorConfig().(sql.ExecutorConfig)
settings := s.ExecutorConfig().(sql.ExecutorConfig).Settings
ie := s.InternalExecutor().(sqlutil.InternalExecutor)
c := s.ExecutorConfig().(sql.ExecutorConfig).SessionInitCache
Expand All @@ -219,18 +234,32 @@ func TestCacheSingleFlight(t *testing.T) {
wgFirstGetAuthInfoCallInProgress.Add(1)
wgForTestComplete.Add(3)

makePlanner := func(opName string) (interface{}, func()) {
return sql.NewInternalPlanner(
opName,
execCfg.DB.NewTxn(ctx, opName),
username.RootUserName(),
&sql.MemoryMetrics{},
s.ExecutorConfig().(*sql.ExecutorConfig),
sessiondatapb.SessionData{},
)
}

go func() {
didReadFromSystemTable := false
_, err := c.GetAuthInfo(ctx, settings, ie, s.DB(), s.ExecutorConfig().(sql.ExecutorConfig).CollectionFactory, testuser, func(
ctx context.Context,
ie sqlutil.InternalExecutor,
userName username.SQLUsername,
makePlanner func(opName string) (interface{}, func()),
settings *cluster.Settings,
) (sessioninit.AuthInfo, error) {
wgFirstGetAuthInfoCallInProgress.Done()
wgForConcurrentReadWrite.Wait()
didReadFromSystemTable = true
return sessioninit.AuthInfo{}, nil
})
},
makePlanner)
require.NoError(t, err)
require.True(t, didReadFromSystemTable)
wgForTestComplete.Done()
Expand All @@ -249,10 +278,13 @@ func TestCacheSingleFlight(t *testing.T) {
ctx context.Context,
ie sqlutil.InternalExecutor,
userName username.SQLUsername,
makePlanner func(opName string) (interface{}, func()),
settings *cluster.Settings,
) (sessioninit.AuthInfo, error) {
didReadFromSystemTable = true
return sessioninit.AuthInfo{}, nil
})
},
makePlanner)
require.NoError(t, err)
require.False(t, didReadFromSystemTable)
wgForTestComplete.Done()
Expand All @@ -270,10 +302,13 @@ func TestCacheSingleFlight(t *testing.T) {
ctx context.Context,
ie sqlutil.InternalExecutor,
userName username.SQLUsername,
makePlanner func(opName string) (interface{}, func()),
settings *cluster.Settings,
) (sessioninit.AuthInfo, error) {
didReadFromSystemTable = true
return sessioninit.AuthInfo{}, nil
})
},
makePlanner)

require.NoError(t, err)
require.True(t, didReadFromSystemTable)
Expand Down
43 changes: 41 additions & 2 deletions pkg/sql/user.go
View file
Open in desktop
Expand Up @@ -14,20 +14,25 @@ import (
"context"
"time"

"github.com/cockroachdb/cockroach/pkg/clusterversion"
"github.com/cockroachdb/cockroach/pkg/keys"
"github.com/cockroachdb/cockroach/pkg/kv"
"github.com/cockroachdb/cockroach/pkg/security"
"github.com/cockroachdb/cockroach/pkg/security/password"
"github.com/cockroachdb/cockroach/pkg/security/username"
"github.com/cockroachdb/cockroach/pkg/settings"
"github.com/cockroachdb/cockroach/pkg/settings/cluster"
"github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb"
"github.com/cockroachdb/cockroach/pkg/sql/catalog/descs"
"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/sessiondata"
"github.com/cockroachdb/cockroach/pkg/sql/sessiondatapb"
"github.com/cockroachdb/cockroach/pkg/sql/sessioninit"
"github.com/cockroachdb/cockroach/pkg/sql/sqlutil"
"github.com/cockroachdb/cockroach/pkg/sql/syntheticprivilege"
"github.com/cockroachdb/cockroach/pkg/util/contextutil"
"github.com/cockroachdb/cockroach/pkg/util/log"
"github.com/cockroachdb/cockroach/pkg/util/log/eventpb"
Expand Down Expand Up @@ -207,6 +212,16 @@ func retrieveSessionInitInfoWithCache(
databaseName string,
) (aInfo sessioninit.AuthInfo, settingsEntries []sessioninit.SettingsCacheEntry, err error) {
if err = func() (retErr error) {
makePlanner := func(opName string) (interface{}, func()) {
return NewInternalPlanner(
opName,
execCfg.DB.NewTxn(ctx, opName),
username.RootUserName(),
&MemoryMetrics{},
execCfg,
sessiondatapb.SessionData{},
)
}
aInfo, retErr = execCfg.SessionInitCache.GetAuthInfo(
ctx,
execCfg.Settings,
Expand All @@ -215,6 +230,7 @@ func retrieveSessionInitInfoWithCache(
execCfg.CollectionFactory,
userName,
retrieveAuthInfo,
makePlanner,
)
if retErr != nil {
return retErr
Expand Down Expand Up @@ -243,7 +259,11 @@ func retrieveSessionInitInfoWithCache(
}

func retrieveAuthInfo(
ctx context.Context, ie sqlutil.InternalExecutor, user username.SQLUsername,
ctx context.Context,
ie sqlutil.InternalExecutor,
user username.SQLUsername,
makePlanner func(opName string) (interface{}, func()),
settings *cluster.Settings,
) (aInfo sessioninit.AuthInfo, retErr error) {
// Use fully qualified table name to avoid looking up "".system.users.
// We use a nil txn as login is not tied to any transaction state, and
Expand Down Expand Up @@ -297,6 +317,23 @@ func retrieveAuthInfo(
aInfo.CanLoginSQL = true
aInfo.CanLoginDBConsole = true
var ok bool

// Check system privilege to see if user can sql login.
planner, cleanup := makePlanner("check-privilege")
defer cleanup()
aa := planner.(AuthorizationAccessor)
hasAdmin, err := aa.HasAdminRole(ctx)
if err != nil {
return aInfo, err
}
if !hasAdmin {
if settings.Version.IsActive(ctx, clusterversion.SystemPrivilegesTable) {
if noSQLLogin := aa.CheckPrivilegeForUser(ctx, syntheticprivilege.GlobalPrivilegeObject, privilege.NOSQLLOGIN, user) == nil; noSQLLogin {
aInfo.CanLoginSQL = false
}
}
}

for ok, err = roleOptsIt.Next(ctx); ok; ok, err = roleOptsIt.Next(ctx) {
row := roleOptsIt.Cur()
option := string(tree.MustBeDString(row[0]))
Expand All @@ -305,7 +342,9 @@ func retrieveAuthInfo(
aInfo.CanLoginSQL = false
aInfo.CanLoginDBConsole = false
}
if option == "NOSQLLOGIN" {
// If the user did not have the NOSQLLOGIN system privilege but has the
// equivalent role option set the flag to false.
if option == "NOSQLLOGIN" && aInfo.CanLoginSQL {
aInfo.CanLoginSQL = false
}

Expand Down