sql,builtins: remove HasAdminRole checks

[rafiss]

These crdb_internal builtins are used for debugging, so should not
require admin access. Instead, we use the VIEWCLUSTERMETADATA or
REPAIRCLUSTERMETADATA privilege.

informs: #114384

---

sql: make admin, root, and node implicit owners of all objects

This lets us remove specific checks for the admin role in many places.

sql: remove unneeded calls to hasAdminRole

All these checks already have a privilege check in the same place, which
already implicitly checks for the admin role.

Release note: None

[blathers-crl]

It looks like your PR touches production code but doesn't add or edit any test code. Did you consider adding tests to your PR?

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.}

[cockroach-teamcity]

This change is 

[dt]

Does “view cluster metadata” really correspond to “read all data regardless of permissions”? I don’t have the written description of it in front of me but just by the name I assume it is eg SHOW RANGES or something not reading any kv?

[rafiss]

my thinking was that technically, you can see arbitrary KVs using SHOW RANGES, so this PR is not a privilege escalation

[dikshant]

We can leave out builtins like scan that can expose data from arbitrary key ranges. Barring that I would be okay with this.

[dt]

You can't see arbitrary KVs: you can see some keys if they were chosen as split keys, but you cannot see values. We already point out that indexed values end up in keys when we talk about domiciling or other data sensitivity questions, but values are generally only visible to those granted access and admins.

[maryliag]

Reviewed 1 of 1 files at r1, 21 of 21 files at r2.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @fqazi and @rafiss)

---

pkg/sql/sem/builtins/builtins.go line 7204 at r2 (raw file):

		),
	),
	"crdb_internal.reset_index_usage_stats": makeBuiltin(

this function is not a read only data, meaning is deleted all stats from the index usage table. I'm not sure we want non-admins to be able to do it.
cc @kevin-v-ngo for some feedback

---

pkg/sql/sem/builtins/builtins.go line 7230 at r2 (raw file):

		},
	),
	"crdb_internal.reset_sql_stats": makeBuiltin(

same comment as above.
@kevin-v-ngo are we okay with non-admins that have the role REPAIRCLUSTERMETADATA be able to reset sql stats?

---

pkg/sql/sem/builtins/builtins.go line 7256 at r2 (raw file):

		},
	),
	"crdb_internal.reset_activity_tables": makeBuiltin(

same as bove

---

pkg/sql/sem/builtins/builtins.go line 7282 at r2 (raw file):

		},
	),
	"crdb_internal.reset_insights_tables": makeBuiltin(

same as above

---

pkg/server/application_api/stmtdiag_test.go line 126 at r2 (raw file):

		"SELECT crdb_internal.request_statement_bundle('SELECT _', 0::FLOAT, 0::INTERVAL, 0::INTERVAL)",
	)
	require.Contains(t, err.Error(), "requesting statement bundle requires VIEWACTIVITY privilege")

did you replace the admin by another option, such as VIEWCLUSTERMETADATA? Just confirming if VIEWACTIVITY is now the only requirement or if there is any other role that would also allow this operation

[rafiss]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @dt, @fqazi, @kevin-v-ngo, and @maryliag)

---

pkg/sql/sem/builtins/builtins.go line 7204 at r2 (raw file):

Previously, maryliag (Marylia Gutierrez) wrote…

this function is not a read only data, meaning is deleted all stats from the index usage table. I'm not sure we want non-admins to be able to do it.
cc @kevin-v-ngo for some feedback

since it's not read-only, that's why i made it use "REPAIRCLUSTERMETADATA" and not "VIEWCLUSTERMETADATA"

the reasoning is that when we are debugging cloud clusters, customers don't want us to use admin, and instead we should only use the minimum set of privileges

[dt]

I have some discomfort with this directionally, and indeed wonder we should be doing the opposite and ensuing all crdb_internal functions require admin?

crdb_internal is explicitly a prefix where we put internal things, and hold them to wholly different quality/safety bars during development to make them more powerful and thus useful in debugging, and cheaper/faster to implement, again with the intent of having more of these tools at our disposal, to be more useful when it comes to debugging. Engineers, myself included, will readily identify a task that would be easier with an internal builtin and add it, even if it is wildly unsafe or violates normal auth checks. Right now we do that "safely" by throwing an admin check on it, but I worry that we could be in for some violated expectations, or at the very least a more burdensome vetting process, if every debugging builtin needs to consider what is the widest audience to which it can be granted safely, are its docs/safety checks/etc all calibrated to that audience, etc. In many ways, just saying "internal == admin" would minimize the likelihood a quick debugging tool ends up causing a security disclosure / TA.

There have, in the past, been crdb_internal builtins that can corrupt a cluster, violate all the norms of our privilege model, or otherwise significantly violate general exceptions around sql-exposed functionality. Making these easier for non-admins to call seems like it introduces a risk of unintended consequences: authors of internal tools generally assume that these are only things used by admins who know exactly what they're doing, but operators may be granting viewclustermetadata to only moderately trained DBAs or SREs on the assumption that the DB would keep them from doing anything too bad.

I also question why we want to make crdb_internal callable by more users? Ideally if non-admins are asking to call these functions, should we be directing them to call non-crdb_internal functions instead of making the internal functions available to non-admins?

[dt]

when we are debugging cloud clusters, customers don't want us to use admin, and instead we should only use the minimum set of privileges

Could we add SHOW syntax for the builtins that we want to use as non-admins instead of opening up the builtins?

[rafiss]

I have some discomfort with this directionally, and indeed wonder we should be doing the opposite and ensuing all crdb_internal functions require admin?

well, this was already was discussed and litigated before. the conclusion where we landed with Abhinav/Duoc/Dikshant and other stakeholders was that we should move away from admin as much as possible (CRDB-17640). this was needed urgently enough that the decision we landed on was to replace all the existing admin checks with checks for one of two new privileges that were explicitly added to replace admin (VIEWCLUSTERMETADATA or REPAIRCLUSTERMETADATA ... or one of the other existing privileges if there actually was already a good fit). since VIEW/REPAIRCLUSTERMETADATA were added purely to replace admin checks, i don't feel that it's accurate to say we are loosening up these endpoints too much.

[dt]

My worry above is that "Abhinav/Duoc/Dikshant and other stakeholders" might not have included "engineers who are adding debugging tools"? As mentioned above, implementation of low-effort/quick debugging is often not treated (for good reasons) the same as user-facing feature work.

existing privileges if there actually was already a good fit

This is what concerns me most here: if we have individual engineers hastily throwing together a quick debugging tool, relying on them to reach for the right privs in the right places increases the chance something going wrong. And when things go wrong in this way it often requires a security disclosure/TA. For functions in the "internal debugging tool" category, that do not get the more rigorous thought and care that goes into user-facing features and their carefully determined access controls, the simplicity of having a single authorization decision -- "is allowed to use potentially-all-powerful internal debug tools?" -- seems like the safest way to not accidentally violate access expectations.

If we really don't want "admin" to be that check, then perhaps "repairclustermetadata" could be the check but it should be the check, that every internal builtin uses, rather than asking them to make nuanced decisions (which could be wrong)?

[dikshant]

If we really don't want "admin" to be that check, then perhaps "repairclustermetadata" could be the check but it should be the check, that every internal builtin uses, rather than asking them to make nuanced decisions (which could be wrong)?

That is what we are suggesting we should do. If the concern is engineers might not know to put it under repairclustermetadata, couldn't we have a test that fails if someone uses admin and have the error message point them towards using repairclustermetadata?

If we need to modify a bunch of other built ins to use repairclustermetadata we can track that as a separate issue.

[dt]

That is what we are suggesting we should do

This change as it stands isn't quite doing that -- making repairclustermetadata the check used by debugging builtins -- though: some things are being changed to check viewclustermetadata while others are checking repairclustermetadata and others are checking other privileges. Asking for nuanced determinations like this on a per-function (or per-flag within a function potentially) basis in debugging tools is what worries me; often there are non-obvious implications of powerful debugging tools, particularly when more than one author is working on them or re-using existing code that might be able to do things that aren't readily apparent (we already have an example, in this diff, where a debugging builtin was moved from admin to VIEWCLUSTERMETADATA even though it grants access to far more than just meta data).

So if we're suggesting we'll replace admin with repair 1:1, that seems reasonably simple/easy for engineers to follow, but only if we actually are doing it 1:1.

[rafiss]

@dt this has been updated so the builtins all use REPAIRCLUSTERMETADATA. i also added a commit that aliases it to REPAIRCLUSTER. (the internal key cannot be changed)

[rafiss]

tftr!

bors r+

[craig]

Build failed:

• Bazel Essential CI (Cockroach)

[rafiss]

bors r-

[rafiss]

I did a find/replace of REPAIRCLUSTERMETADATA/REPAIRCLUSTER in comments, variables names, and test expectations.

bors r+

[craig]

Build succeeded:

• Bazel Essential CI (Cockroach)
• check_generated_code
• docker_image_amd64
• examples_orms
• macos_amd64_build
• macos_arm64_build
• windows_build