sql, server: observability replace role options with system privileges

[Santamaura]

This patch introduces system privileges that exist currently as
role options. For observability they are the following: VIEWACTIVITY,
VIEWACTIVTYREDACTED, VIEWCLUSTERSETTING, CANCELQUERY and NOSQLLOGIN.
Users should now do GRANT SYSTEM privilege TO role; instead of
using role options.
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 (sql change): introduced
VIEWACTIVITY, VIEWACTIVITYREDACTED,
VIEWCLUSTERSETTING, CANCELQUERY,
and NOSQLLOGIN as system privileges.

[cockroach-teamcity]

This change is 

[rafiss]

drive-by comment -- i feel like this change deserves a release note

[RichardJCai]

Reviewed all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @koorosh, @Santamaura, and @zachlite)

---

-- commits line 6 at r2:
I'd add a note that you should now do GRANT SYSTEM ... TO ... instead of using the role options.

---

pkg/server/admin.go line 1860 at r2 (raw file):

						privilege.VIEWCLUSTERSETTING, privilege.MODIFYCLUSTERSETTING)
				}
			} else {

nit: Don't need the else if we're just returning

---

pkg/server/admin.go line 3474 at r2 (raw file):

						privilege.VIEWACTIVITY)
				}
			} else {

nit: Don't need the else if we're just returning

Ditto for other cases below

---

pkg/server/admin.go line 3642 at r2 (raw file):

// privilege provided.
func (c *adminPrivilegeChecker) checkHasSystemPrivilege(
	ctx context.Context, user username.SQLUsername, privilege string,

can privilege string become privilege privilege.Kind?

I'm kind of okay with this initially but this is not ideal since it'll not use the cache which I'm implementing in a separate PR. Can you add a todo to update this to do the below?

Also can you note what the issue was with creating a planner.

desc := syntheticprivilege.GlobalPrivilegeObject.GetPrivilegeDescriptor(ctx, ie)
desc.CheckPrivilege()
...

[koorosh]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @Santamaura and @zachlite)

a discussion (no related file):
As far as I understand, system privileges should replace existing role options, but current logic requires that user has both assigned role options and system privileges.
I suggest having the following workflow:

• check the system privileges table first (for supported versions) and skip role options validation if user has explicitly defined system privilege; In this case system privileges take precedence over role options.
• if the user doesn't have appropriate privilege then fallback to role options validation;

---

[RichardJCai]

One more thing, this doesn't account for inheritance.

[Santamaura]

Added

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @koorosh, @RichardJCai, and @zachlite)

a discussion (no related file):

Previously, koorosh (Andrii Vorobiov) wrote…

As far as I understand, system privileges should replace existing role options, but current logic requires that user has both assigned role options and system privileges.
I suggest having the following workflow:

• check the system privileges table first (for supported versions) and skip role options validation if user has explicitly defined system privilege; In this case system privileges take precedence over role options.
• if the user doesn't have appropriate privilege then fallback to role options validation;

Moved stuff around so that the logic reflects this idea.

---

---

-- commits line 6 at r2:

Previously, RichardJCai (Richard Cai) wrote…

I'd add a note that you should now do GRANT SYSTEM ... TO ... instead of using the role options.

Done

---

pkg/server/admin.go line 3474 at r2 (raw file):

Previously, RichardJCai (Richard Cai) wrote…

nit: Don't need the else if we're just returning

Ditto for other cases below

Done.

---

pkg/server/admin.go line 3642 at r2 (raw file):

Previously, RichardJCai (Richard Cai) wrote…

can privilege string become privilege privilege.Kind?

I'm kind of okay with this initially but this is not ideal since it'll not use the cache which I'm implementing in a separate PR. Can you add a todo to update this to do the below?

Also can you note what the issue was with creating a planner.

desc := syntheticprivilege.GlobalPrivilegeObject.GetPrivilegeDescriptor(ctx, ie)
desc.CheckPrivilege()
...

Per our discussion, switched to using NewInternalPlanner wrapped in a function so this is no longer relevant.

---

pkg/server/admin.go line 3647 at r2 (raw file):

Previously, RichardJCai (Richard Cai) wrote…

One more thing, this doesn't account for inheritance.

Ack

[koorosh]

Reviewed 1 of 12 files at r1, 2 of 5 files at r2, 12 of 12 files at r3, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @koorosh, @RichardJCai, @Santamaura, and @zachlite)

---

pkg/server/admin.go line 1838 at r3 (raw file):

		if s.st.Version.IsActive(ctx, clusterversion.SystemPrivilegesTable) {
			hasView = s.checkHasSystemPrivilege(ctx, user, privilege.VIEWCLUSTERSETTING)
			hasModify = s.checkHasSystemPrivilege(ctx, user, privilege.MODIFYCLUSTERSETTING)

nit. It doesn't relate to this PR specifically, but it might be useful to introduce an extended version of heckHasSystemPrivilege to check the list of privileges and return a single result if some of the provided privileges exist (ie checkHasSomeSystemPrivileges)

hasPrivilege := s.checkHasSomeSystemPrivileges(ctx, user, []privilege.Kind{privilege.VIEWCLUSTERSETTING, privilege.MODIFYCLUSTERSETTING})
if hasPrivilege {
  ...
}

---

pkg/sql/crdb_internal.go line 1565 at r3 (raw file):

			hasView := false
			if p.ExecCfg().Settings.Version.IsActive(ctx, clusterversion.SystemPrivilegesTable) {
				hasModify = p.CheckPrivilege(ctx, syntheticprivilege.GlobalPrivilegeObject, privilege.MODIFYCLUSTERSETTING) == nil

Ignored possible errors here. p.CheckPrivilege can return error that is not handled.

[Santamaura]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @koorosh, @RichardJCai, and @zachlite)

---

pkg/server/admin.go line 1838 at r3 (raw file):

Previously, koorosh (Andrii Vorobiov) wrote…

nit. It doesn't relate to this PR specifically, but it might be useful to introduce an extended version of heckHasSystemPrivilege to check the list of privileges and return a single result if some of the provided privileges exist (ie checkHasSomeSystemPrivileges)

hasPrivilege := s.checkHasSomeSystemPrivileges(ctx, user, []privilege.Kind{privilege.VIEWCLUSTERSETTING, privilege.MODIFYCLUSTERSETTING})
if hasPrivilege {
  ...
}

I could see that being potentially useful but maybe it should be added as needed.

---

pkg/sql/crdb_internal.go line 1565 at r3 (raw file):

Previously, koorosh (Andrii Vorobiov) wrote…

Ignored possible errors here. p.CheckPrivilege can return error that is not handled.

p.CheckPrivilege only returns an error or nil so regardless of how the logic is shuffled around we can't return prematurely here with the error since we need to fallback to checking the role options

[RichardJCai]

Nice work!

[RichardJCai]

I'm surprised this changed.

[Santamaura]

Yeah still not sure why it had to be updated

[RichardJCai]

nit: remove this line

[Santamaura]

Removed

[Santamaura]

TFTRs

[Santamaura]

bors r+

[craig]

Build failed (retrying...):

• GitHub CI (Cockroach)

[craig]

Build succeeded:

• GitHub CI (Cockroach)