Skip to content

Commit 05e3225

Browse files
SantamauraSantamaura
committed
sql, server: observability replace role options with system privileges
This patch introduces system privileges that exist currently as role options. For observability they are the following: VIEWACTIVITY, VIEWACTIVTYREDACTED and VIEWCLUSTERSETTING. Brand new system privileges for observability will be introduced in a subsequent PR. Any additional gating with these system privileges beyond the basic ones made in this PR will be done in a subsequent PR. Resolves #83843 Release note: None
1 parent 262dabe commit 05e3225

File tree

12 files changed

+330
-33
lines changed

12 files changed

+330
-33
lines changed

‎pkg/server/BUILD.bazel

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -181,6 +181,7 @@ go_library(
181181
"//pkg/sql/pgwire/pgerror",
182182
"//pkg/sql/pgwire/pgwirecancel",
183183
"//pkg/sql/physicalplan",
184+
"//pkg/sql/privilege",
184185
"//pkg/sql/querycache",
185186
"//pkg/sql/rangeprober",
186187
"//pkg/sql/roleoption",

‎pkg/server/admin.go

Lines changed: 92 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@ import (
2424

2525
"github.com/cockroachdb/apd/v3"
2626
"github.com/cockroachdb/cockroach/pkg/base"
27+
"github.com/cockroachdb/cockroach/pkg/clusterversion"
2728
"github.com/cockroachdb/cockroach/pkg/config/zonepb"
2829
"github.com/cockroachdb/cockroach/pkg/jobs"
2930
"github.com/cockroachdb/cockroach/pkg/jobs/jobspb"
@@ -46,6 +47,7 @@ import (
4647
"github.com/cockroachdb/cockroach/pkg/sql/catalog/colinfo"
4748
"github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb"
4849
"github.com/cockroachdb/cockroach/pkg/sql/parser"
50+
"github.com/cockroachdb/cockroach/pkg/sql/privilege"
4951
"github.com/cockroachdb/cockroach/pkg/sql/roleoption"
5052
"github.com/cockroachdb/cockroach/pkg/sql/sem/tree"
5153
"github.com/cockroachdb/cockroach/pkg/sql/sessiondata"
@@ -1839,9 +1841,27 @@ func (s *adminServer) Settings(
18391841
}
18401842

18411843
if !hasModify && !hasView {
1842-
return nil, status.Errorf(
1843-
codes.PermissionDenied, "this operation requires either %s or %s role options",
1844-
roleoption.VIEWCLUSTERSETTING, roleoption.MODIFYCLUSTERSETTING)
1844+
if s.st.Version.IsActive(ctx, clusterversion.SystemPrivilegesTable) {
1845+
hasViewSystemPrivilege, err := s.checkHasSystemPrivilege(ctx, user, "VIEWCLUSTERSETTING")
1846+
if err != nil {
1847+
return nil, err
1848+
}
1849+
1850+
hasModifystemPrivilege, err := s.checkHasSystemPrivilege(ctx, user, "MODIFYCLUSTERSETTING")
1851+
if err != nil {
1852+
return nil, err
1853+
}
1854+
1855+
if !hasViewSystemPrivilege && !hasModifystemPrivilege {
1856+
return nil, status.Errorf(
1857+
codes.PermissionDenied, "this operation requires either %s or %s system privileges",
1858+
privilege.VIEWCLUSTERSETTING, privilege.MODIFYCLUSTERSETTING)
1859+
}
1860+
} else {
1861+
return nil, status.Errorf(
1862+
codes.PermissionDenied, "this operation requires either %s or %s role options",
1863+
roleoption.VIEWCLUSTERSETTING, roleoption.MODIFYCLUSTERSETTING)
1864+
}
18451865
}
18461866
}
18471867

@@ -3411,6 +3431,7 @@ func (s *adminServer) dialNode(
34113431
// have admin privileges.
34123432
type adminPrivilegeChecker struct {
34133433
ie *sql.InternalExecutor
3434+
st *cluster.Settings
34143435
}
34153436

34163437
// requireAdminUser's error return is a gRPC error.
@@ -3440,9 +3461,21 @@ func (c *adminPrivilegeChecker) requireViewActivityPermission(ctx context.Contex
34403461
}
34413462

34423463
if !hasViewActivity {
3443-
return status.Errorf(
3444-
codes.PermissionDenied, "this operation requires the %s role option",
3445-
roleoption.VIEWACTIVITY)
3464+
hasSystemPrivilege, err := c.checkHasSystemPrivilege(ctx, userName, "VIEWACTIVITY")
3465+
if err != nil {
3466+
return err
3467+
}
3468+
if c.st.Version.IsActive(ctx, clusterversion.SystemPrivilegesTable) {
3469+
if !hasSystemPrivilege {
3470+
return status.Errorf(
3471+
codes.PermissionDenied, "this operation requires the %s system privilege",
3472+
privilege.VIEWACTIVITY)
3473+
}
3474+
} else {
3475+
return status.Errorf(
3476+
codes.PermissionDenied, "this operation requires the %s role option",
3477+
roleoption.VIEWACTIVITY)
3478+
}
34463479
}
34473480
}
34483481
return nil
@@ -3462,13 +3495,29 @@ func (c *adminPrivilegeChecker) requireViewActivityOrViewActivityRedactedPermiss
34623495
return serverError(ctx, err)
34633496
}
34643497

3465-
if !hasViewActivity {
3466-
hasViewActivityRedacted, err := c.hasRoleOption(ctx, userName, roleoption.VIEWACTIVITYREDACTED)
3467-
if err != nil {
3468-
return serverError(ctx, err)
3469-
}
3498+
hasViewActivityRedacted, err := c.hasRoleOption(ctx, userName, roleoption.VIEWACTIVITYREDACTED)
3499+
if err != nil {
3500+
return serverError(ctx, err)
3501+
}
3502+
3503+
if !hasViewActivity && !hasViewActivityRedacted {
3504+
if c.st.Version.IsActive(ctx, clusterversion.SystemPrivilegesTable) {
3505+
hasVASystemPrivilege, err := c.checkHasSystemPrivilege(ctx, userName, "VIEWACTIVITY")
3506+
if err != nil {
3507+
return err
3508+
}
3509+
3510+
hasVARSystemPrivilege, err := c.checkHasSystemPrivilege(ctx, userName, "VIEWACTIVITYREDACTED")
3511+
if err != nil {
3512+
return err
3513+
}
34703514

3471-
if !hasViewActivityRedacted {
3515+
if !hasVASystemPrivilege && !hasVARSystemPrivilege {
3516+
return status.Errorf(
3517+
codes.PermissionDenied, "this operation requires the %s or %s system privilege",
3518+
privilege.VIEWACTIVITY, privilege.VIEWACTIVITYREDACTED)
3519+
}
3520+
} else {
34723521
return status.Errorf(
34733522
codes.PermissionDenied, "this operation requires the %s or %s role options",
34743523
roleoption.VIEWACTIVITY, roleoption.VIEWACTIVITYREDACTED)
@@ -3499,6 +3548,17 @@ func (c *adminPrivilegeChecker) requireViewActivityAndNoViewActivityRedactedPerm
34993548
codes.PermissionDenied, "this operation requires %s role option and is not allowed for %s role option",
35003549
roleoption.VIEWACTIVITY, roleoption.VIEWACTIVITYREDACTED)
35013550
}
3551+
if c.st.Version.IsActive(ctx, clusterversion.SystemPrivilegesTable) {
3552+
hasVARSystemPrivilege, err := c.checkHasSystemPrivilege(ctx, userName, "VIEWACTIVITYREDACTED")
3553+
if err != nil {
3554+
return err
3555+
}
3556+
if hasVARSystemPrivilege {
3557+
return status.Errorf(
3558+
codes.PermissionDenied, "this operation requires %s system privilege and is not allowed for %s system privilege",
3559+
privilege.VIEWACTIVITY, privilege.VIEWACTIVITYREDACTED)
3560+
}
3561+
}
35023562
return c.requireViewActivityPermission(ctx)
35033563
}
35043564
return nil
@@ -3575,6 +3635,26 @@ func (c *adminPrivilegeChecker) hasRoleOption(
35753635
return bool(dbDatum), nil
35763636
}
35773637

3638+
// checkHasSystemPrivilege is a helper function which executes
3639+
// a query to see if rows return non-nil based on the
3640+
// privilege provided.
3641+
func (c *adminPrivilegeChecker) checkHasSystemPrivilege(
3642+
ctx context.Context, user username.SQLUsername, privilege string,
3643+
) (bool, error) {
3644+
row, err := c.ie.QueryRowEx(
3645+
ctx, "check-has-system-privilege", nil, /* txn */
3646+
sessiondata.InternalExecutorOverride{User: username.RootUserName()},
3647+
`SELECT * FROM system.privileges WHERE username = $1 AND privileges @> ARRAY[$2]`,
3648+
user, privilege)
3649+
if err != nil {
3650+
return false, err
3651+
}
3652+
if row == nil {
3653+
return false, nil
3654+
}
3655+
return true, nil
3656+
}
3657+
35783658
var errRequiresAdmin = status.Error(codes.PermissionDenied, "this operation requires admin privilege")
35793659

35803660
func errRequiresRoleOption(option roleoption.Option) error {

‎pkg/server/admin_test.go

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,7 @@ import (
2828
"time"
2929

3030
"github.com/cockroachdb/cockroach/pkg/base"
31+
"github.com/cockroachdb/cockroach/pkg/clusterversion"
3132
"github.com/cockroachdb/cockroach/pkg/config/zonepb"
3233
"github.com/cockroachdb/cockroach/pkg/jobs"
3334
"github.com/cockroachdb/cockroach/pkg/jobs/jobspb"
@@ -2769,6 +2770,7 @@ func TestAdminPrivilegeChecker(t *testing.T) {
27692770

27702771
underTest := &adminPrivilegeChecker{
27712772
ie: s.InternalExecutor().(*sql.InternalExecutor),
2773+
st: s.ClusterSettings(),
27722774
}
27732775

27742776
withAdmin, err := username.MakeSQLUsernameFromPreNormalizedStringChecked("withadmin")
@@ -2809,6 +2811,34 @@ func TestAdminPrivilegeChecker(t *testing.T) {
28092811
},
28102812
},
28112813
}
2814+
// test system privileges if valid version
2815+
if s.ClusterSettings().Version.IsActive(ctx, clusterversion.SystemPrivilegesTable) {
2816+
sqlDB.Exec(t, "CREATE USER withvasystemprivilege")
2817+
sqlDB.Exec(t, "GRANT SYSTEM VIEWACTIVITY TO withvasystemprivilege")
2818+
sqlDB.Exec(t, "CREATE USER withvaredactedsystemprivilege")
2819+
sqlDB.Exec(t, "GRANT SYSTEM VIEWACTIVITYREDACTED TO withvaredactedsystemprivilege")
2820+
sqlDB.Exec(t, "CREATE USER withvaandredactedsystemprivilege")
2821+
sqlDB.Exec(t, "GRANT SYSTEM VIEWACTIVITY TO withvaandredactedsystemprivilege")
2822+
sqlDB.Exec(t, "GRANT SYSTEM VIEWACTIVITYREDACTED TO withvaandredactedsystemprivilege")
2823+
2824+
withVaSystemPrivilege, err := username.MakeSQLUsernameFromPreNormalizedStringChecked("withvasystemprivilege")
2825+
require.NoError(t, err)
2826+
withVaRedactedSystemPrivilege, err := username.MakeSQLUsernameFromPreNormalizedStringChecked("withvaredactedsystemprivilege")
2827+
require.NoError(t, err)
2828+
withVaAndRedactedSystemPrivilege, err := username.MakeSQLUsernameFromPreNormalizedStringChecked("withvaandredactedsystemprivilege")
2829+
require.NoError(t, err)
2830+
2831+
tests[0].usernameWantErr[withVaSystemPrivilege] = false
2832+
tests[1].usernameWantErr[withVaSystemPrivilege] = false
2833+
tests[2].usernameWantErr[withVaSystemPrivilege] = false
2834+
tests[0].usernameWantErr[withVaRedactedSystemPrivilege] = true
2835+
tests[1].usernameWantErr[withVaRedactedSystemPrivilege] = false
2836+
tests[2].usernameWantErr[withVaRedactedSystemPrivilege] = true
2837+
tests[0].usernameWantErr[withVaAndRedactedSystemPrivilege] = false
2838+
tests[1].usernameWantErr[withVaRedactedSystemPrivilege] = false
2839+
tests[2].usernameWantErr[withVaRedactedSystemPrivilege] = true
2840+
2841+
}
28122842
for _, tt := range tests {
28132843
for userName, wantErr := range tt.usernameWantErr {
28142844
t.Run(fmt.Sprintf("%s-%s", tt.name, userName), func(t *testing.T) {

‎pkg/server/server.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -715,7 +715,7 @@ func NewServer(cfg Config, stopper *stop.Stopper) (*Server, error) {
715715

716716
lateBoundServer := &Server{}
717717
// TODO(tbg): give adminServer only what it needs (and avoid circular deps).
718-
adminAuthzCheck := &adminPrivilegeChecker{ie: internalExecutor}
718+
adminAuthzCheck := &adminPrivilegeChecker{ie: internalExecutor, st: st}
719719
sAdmin := newAdminServer(lateBoundServer, adminAuthzCheck, internalExecutor)
720720

721721
// These callbacks help us avoid a dependency on gossip in httpServer.

‎pkg/server/status_test.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2297,7 +2297,7 @@ func TestListActivitySecurity(t *testing.T) {
22972297
ts := s.(*TestServer)
22982298
defer ts.Stopper().Stop(ctx)
22992299

2300-
expectedErrNoPermission := "this operation requires the VIEWACTIVITY or VIEWACTIVITYREDACTED role options"
2300+
expectedErrNoPermission := "this operation requires the VIEWACTIVITY or VIEWACTIVITYREDACTED system privilege"
23012301
contentionMsg := &serverpb.ListContentionEventsResponse{}
23022302
flowsMsg := &serverpb.ListDistSQLFlowsResponse{}
23032303
getErrors := func(msg protoutil.Message) []serverpb.ListActivityError {

‎pkg/sql/authorization.go

Lines changed: 16 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@ import (
1414
"context"
1515
"fmt"
1616

17+
"github.com/cockroachdb/cockroach/pkg/clusterversion"
1718
"github.com/cockroachdb/cockroach/pkg/keys"
1819
"github.com/cockroachdb/cockroach/pkg/kv"
1920
"github.com/cockroachdb/cockroach/pkg/security/username"
@@ -35,6 +36,7 @@ import (
3536
"github.com/cockroachdb/cockroach/pkg/sql/sessiondata"
3637
"github.com/cockroachdb/cockroach/pkg/sql/sessioninit"
3738
"github.com/cockroachdb/cockroach/pkg/sql/sqlutil"
39+
"github.com/cockroachdb/cockroach/pkg/sql/syntheticprivilege"
3840
"github.com/cockroachdb/cockroach/pkg/util"
3941
"github.com/cockroachdb/cockroach/pkg/util/log"
4042
"github.com/cockroachdb/cockroach/pkg/util/mon"
@@ -856,12 +858,20 @@ func (p *planner) HasViewActivityOrViewActivityRedactedRole(ctx context.Context)
856858
if err != nil {
857859
return hasViewActivity, err
858860
}
859-
if !hasViewActivity {
860-
hasViewActivityRedacted, err := p.HasRoleOption(ctx, roleoption.VIEWACTIVITYREDACTED)
861-
if err != nil {
862-
return hasViewActivityRedacted, err
863-
}
864-
if !hasViewActivityRedacted {
861+
hasViewActivityRedacted, err := p.HasRoleOption(ctx, roleoption.VIEWACTIVITYREDACTED)
862+
if err != nil {
863+
return hasViewActivityRedacted, err
864+
}
865+
if !hasViewActivity && !hasViewActivityRedacted {
866+
if p.ExecCfg().Settings.Version.IsActive(ctx, clusterversion.SystemPrivilegesTable) {
867+
noViewActivityErr := p.CheckPrivilege(ctx, syntheticprivilege.GlobalPrivilegeObject, privilege.VIEWACTIVITY)
868+
noViewActivityRedactedErr := p.CheckPrivilege(ctx, syntheticprivilege.GlobalPrivilegeObject, privilege.VIEWACTIVITYREDACTED)
869+
if noViewActivityErr != nil && noViewActivityRedactedErr != nil {
870+
// a number of tests fail if an error is returned here.
871+
// nolint:returnerrcheck
872+
return false, nil
873+
}
874+
} else {
865875
return false, nil
866876
}
867877
}

‎pkg/sql/crdb_internal.go

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1570,11 +1570,15 @@ CREATE TABLE crdb_internal.cluster_settings (
15701570
if !hasModify && !hasView {
15711571
if p.ExecCfg().Settings.Version.IsActive(ctx, clusterversion.SystemPrivilegesTable) {
15721572
// We check for EITHER the MODIFYCLUSTERSETTING or VIEWCLUSTERSETTING
1573-
// role option OR the MODIFYCLUSTERSETTING system cluster privilege.
1573+
// role option OR the MODIFYCLUSTERSETTING or VIEWCLUSTERSETTING system cluster privilege.
15741574
// We return the error for "system cluster privilege" due to
15751575
// the long term goal of moving away from coarse-grained role options.
1576-
if err := p.CheckPrivilege(ctx, syntheticprivilege.GlobalPrivilegeObject, privilege.MODIFYCLUSTERSETTING); err != nil {
1577-
return err
1576+
hasNoModifyErr := p.CheckPrivilege(ctx, syntheticprivilege.GlobalPrivilegeObject, privilege.MODIFYCLUSTERSETTING)
1577+
hasNoViewErr := p.CheckPrivilege(ctx, syntheticprivilege.GlobalPrivilegeObject, privilege.VIEWCLUSTERSETTING)
1578+
if hasNoModifyErr != nil && hasNoViewErr != nil {
1579+
return pgerror.Newf(pgcode.InsufficientPrivilege,
1580+
"only users with either %s or %s system privileges are allowed to read "+
1581+
"crdb_internal.cluster_settings", privilege.MODIFYCLUSTERSETTING, privilege.VIEWCLUSTERSETTING)
15781582
}
15791583
} else {
15801584
return pgerror.Newf(pgcode.InsufficientPrivilege,

0 commit comments

Comments
 (0)