Skip to content

Commit

Permalink
sql, server: regulate access to remaining observability features
Browse files Browse the repository at this point in the history
This change will control access to various observability
features based on system privileges including the following:
- admin ui databases/tables/schema endpoints requires admin or VIEWACTIVITY
- EXPERIMENTAL_AUDIT requires admin or MODIFYCLUSTERSETTING
- sql login requires not having NOSQLLOGIN or the equivalent
role option

Resolves: #83848, #83863, #83862

Release note (security update): Change requirements to access some
observability features. Databases/tables/schema endpoints for
admin ui require admin or VIEWACTIVITY. EXPERIMENTAL_AUDIT
requires admin or MODIFYCLUSTERSETTING. SQL login requires not
having NOSQLLOGIN or the equivalent role option.
  • Loading branch information
Santamaura committed Aug 11, 2022
1 parent 47d34c9 commit 9e5c7a0
Show file tree
Hide file tree
Showing 9 changed files with 143 additions and 16 deletions.
22 changes: 20 additions & 2 deletions pkg/server/admin.go
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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

0 comments on commit 9e5c7a0

Please sign in to comment.