kv: reject admin KV requests from tenant SQL processes#52592
kv: reject admin KV requests from tenant SQL processes#52592craig[bot] merged 1 commit intocockroachdb:masterfrom
Conversation
|
This change is |
tbg
left a comment
There was a problem hiding this comment.
Reviewed 2 of 2 files at r1, 12 of 12 files at r2, 8 of 8 files at r3.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @nvanbenschoten)
pkg/ccl/logictestccl/testdata/logic_test/tenant_unsupported, line 84 at r3 (raw file):
# Cannot perform operations that issue Admin requests. # # TODO DURING REVIEW: we could wrap these up in nicer errors. It's unclear
There's some utility to making this tell users to contact us, as they should not be able to hit these. We want to avoid users hitting this. Maybe use errorutil.UnsupportedWithMultiTenancy?
pkg/rpc/auth_tenant.go, line 120 at r3 (raw file):
func (a tenantAuth) authBatch(tenID roachpb.TenantID, args *roachpb.BatchRequest) error { // Admin batch requests are not permitted. if args.IsAdmin() {
Is there a way to keep this function allowlist-style? I.e. "request only has flags X" as opposed to "does not have Z". Actually, don't we want to lock this down to specific request types? You can only Get, CPut, Scan, etc and everything else is out. Our flag stuff is a mess, I wouldn't want to rely on it too much for authz.
pkg/sql/logictest/testdata/logic_test/select_index_span_ranges, line 1 at r3 (raw file):
# LogicTest: !3node-tenant
Issue no?
Fixes cockroachdb#52360. This commit adds a new restriction to the tenantAuth policy that it accepts no "Admin" KV requests. This prevents tenants from splitting ranges, merging range, rebalancing ranges, or issuing any other KV requests with the `isAdmin` flag that could dictate KV-level distribution decisions. This further mitigates the impact that a compromised tenant SQL process could have on the rest of the cluster.
730a44e to
3018397
Compare
nvb
left a comment
There was a problem hiding this comment.
TFTR!
Reviewable status:
pkg/ccl/logictestccl/testdata/logic_test/tenant_unsupported, line 84 at r3 (raw file):
Previously, tbg (Tobias Grieger) wrote…
There's some utility to making this tell users to contact us, as they should not be able to hit these. We want to avoid users hitting this. Maybe use
errorutil.UnsupportedWithMultiTenancy?
Done.
pkg/rpc/auth_tenant.go, line 120 at r3 (raw file):
Previously, tbg (Tobias Grieger) wrote…
Is there a way to keep this function allowlist-style? I.e. "request only has flags X" as opposed to "does not have Z". Actually, don't we want to lock this down to specific request types? You can only Get, CPut, Scan, etc and everything else is out. Our flag stuff is a mess, I wouldn't want to rely on it too much for authz.
Done.
pkg/sql/logictest/testdata/logic_test/select_index_span_ranges, line 1 at r3 (raw file):
Previously, tbg (Tobias Grieger) wrote…
Issue no?
Unless we allow manual splits, we're never going to be able to run this file. It's only a single test but performs 6 manual splits. So there's no issue to reference.
|
bors r+ |
|
Build succeeded: |
Fixes #52360.
This commit adds a new restriction to the tenantAuth policy that it accepts no "Admin" KV requests. This prevents tenants from splitting ranges, merging range, rebalancing ranges, or issuing any other KV requests with the
isAdminflag that could dictate KV-level distribution decisions.This further mitigates the impact that a compromised tenant SQL process could have on the rest of the cluster.