sql: enforce admin role for resetting sql stats and index usage stats #79810
Conversation
|
This change is |
pkg/sql/sem/builtins/builtins.go
Outdated
| return nil, err | ||
| } | ||
| if !isAdmin { | ||
| return nil, errors.New("crdb_internal.reset_index_usage_stats() require admin privilege") |
There was a problem hiding this comment.
nit: requires admin privilege
pkg/sql/sem/builtins/builtins.go
Outdated
| return nil, err | ||
| } | ||
| if !isAdmin { | ||
| return nil, errors.New("crdb_internal.reset_sql_stats() require admin privilege") |
| // TestResetIndexUsageStatsOnRemoteSQLNode asserts that the built-in for | ||
| // resetting index usage statistics works when it's being set up on a remote | ||
| // node via DistSQL. | ||
| func TestResetIndexUsageStatsOnRemoteSQLNode(t *testing.T) { |
There was a problem hiding this comment.
I'm unfamiliar with the terminology here - does remote node mean not gateway?
I don't think we have a test here to run crdb_internal.reset_index_usage_stats() on a gateway node either.
There was a problem hiding this comment.
Do we really want to restrict this to admin role only? Doesn't VIEWACTIVITY and VIEWACTIVITYREDACTED should be able to do this?
Otherwise we might want to hide all the reset options on the ui if that is indeed the decision.
@kevin-v-ngo thoughts?
Reviewed all commit messages.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @Azhng, @maryliag, and @RichardJCai)
pkg/sql/sem/builtins/builtins.go, line 6425 at r1 (raw file):
tree.FunctionProperties{ Category: categorySystemInfo, DistsqlBlocklist: true, // applicable only on the gateway
nit: DistSQLBlocklist
There was a problem hiding this comment.
@RichardJCai the reset_index_usage_stats() is tested on the tenant_status_test.go.
I'm unfamiliar with the terminology here - does remote node mean not gateway?
Yes. This was because previously, DistSQL can potentially push down the filter, which means the builtin can be executed on the remote node. Now this behavior is disallowed by setting DistsqlBlocklist to true. So it's a case we don't have to worry about.
Reviewable status:
pkg/sql/sem/builtins/builtins.go, line 6425 at r1 (raw file):
Previously, maryliag (Marylia Gutierrez) wrote…
nit: DistSQLBlocklist
Hmm this is an existing field owned by a different team. I'll make a separate PR for this change, since this will change all the builtins.
pkg/sql/sem/builtins/builtins.go, line 6436 at r1 (raw file):
Previously, RichardJCai (Richard Cai) wrote…
nit: requires admin privilege
Done.
pkg/sql/sem/builtins/builtins.go, line 6465 at r1 (raw file):
Previously, RichardJCai (Richard Cai) wrote…
ditto
Done.
Resolves cockroachdb#79688 Previously, SQL Stats and Index Usage Stats can be reset through SQL CLI using crdb_internal.reset_sql_stats() and crdb_internal.reset_index_usage_stats() builtins. However, these two builtins were not checking for users admin role. Hence, any user can reset SQL Stats and Index Usage Stats. This commit enforces the permission check. Release note (security update): crdb_internal.reset_sql_stats() and crdb_internal.reset_index_usage_stats() builtins now check if user has admin role.
There was a problem hiding this comment.
Reviewed 2 of 5 files at r1, 3 of 3 files at r2, all commit messages.
Reviewable status:
|
bors r=maryliag |
|
Build failed (retrying...): |
|
Build succeeded: |
|
blather backport 22.1.0 |
|
blathers backport release-22.1.0 |
Resolves #79688
Previously, SQL Stats and Index Usage Stats can be reset through SQL CLI
using crdb_internal.reset_sql_stats() and
crdb_internal.reset_index_usage_stats() builtins. However, these two
builtins were not checking for users admin role. Hence, any user can
reset SQL Stats and Index Usage Stats.
This commit enforces the permission check.
Release note (security update): crdb_internal.reset_sql_stats() and
crdb_internal.reset_index_usage_stats() builtins now check if user has
admin role.